Add typings to the art resizer module #4649

Serene-Arc · 2023-01-22T02:14:06Z

Continuing my effort to type the modules of beets, here's another module typed. Few comments:

Pillow's Image module is imported at the top so I could use the typings. There are other duplicate imports in the code; the tests run so it doesn't cause problems there but I don't know why it was done in the first place so worth noting
- If it's fine, then there are duplicate imports that should be removed
@wisp3rwind said that beets considers every path beyond the core to be a string, or whatever needed by external APIs. I have here typed them to be AnyStr which includes both bytes and strings for maximum breadth, but if they are truly strings, then this can be tightened.

wisp3rwind · 2023-02-12T14:05:46Z

Pillow's Image module is imported at the top so I could use the typings. There are other duplicate imports in the code; the tests run so it doesn't cause problems there but I don't know why it was done in the first place so worth noting

If it's fine, then there are duplicate imports that should be removed

The purpose of the delayed imports is to only require these libraries if they actually end up being used. See the __init__ methods on the two backend classes. Thus, this should stay as is. For typing, we could maybe hide this behind a https://docs.python.org/3/library/typing.html#typing.TYPE_CHECKING check?

@wisp3rwind said that beets considers every path beyond the core to be a string, or whatever needed by external APIs. I have here typed them to be AnyStr which includes both bytes and strings for maximum breadth, but if they are truly strings, then this can be tightened.

Can be both, since on Linux, the bytes are directly passed on (via syspath returning early in that case).

ybnd

Did some local tests/type checks on this branch, highlighted some problems.

Other than the comments there are still a few minor problems that need to be addressed too:

Some Optional values where None is not taken into account
Some str vs. bytes mismatches

beets/util/artresizer.py

ybnd · 2023-05-23T18:30:37Z

beets/util/artresizer.py

@@ -66,7 +70,7 @@ class LocalBackendNotAvailableError(Exception):

 class LocalBackend:
    @classmethod
-    def available(cls):
+    def available(cls) -> bool:
        try:
            cls.version()


We should have an abstract method LocalBackend.version() for this line to pass the type check.

Is that in scope for this typing PR or a comment on something that needs to be done at another time?

Up to you, but since this just means adding a

def version(cls) -> ...: raise NotImplementedError()

I think it's not such a big change that would make this PR intractable. (Actually making LocalBackend an abc.ABC and using the @abstractmethod decorator would be some more work, but I guess with the above stopgap solution we already get quite far, and the @abstractmethod way would require it anyway.)

ybnd · 2023-05-23T18:38:27Z

beets/util/artresizer.py

@@ -84,7 +88,7 @@ class IMBackend(LocalBackend):
    _legacy = None

    @classmethod
-    def version(cls):
+    def version(cls) -> Optional[Union[object, Tuple[int, int, int]]]:


This may be out of scope for a typing PR, but I think this annotation is a bit much; if anything it highlights that the version logic is a bit hard to work with.

I'd suggest

Some other way to handle "no version available" instead of the raw object instance

None would be way more clear

Could allow for that by changing the check below to not hasattr(...) instead of ... is None

Replace the "raw tuple" with a NamedTuple

All in all this would simplify those annotations by a lot:

class Version(NamedTuple): major: int minor: int patch: int class IMBacked(LocalBackend): @classmethod def version(cls) -> Optional[Version]: ...

I think it is out of scope. I don't think I should be changing the logic of the methods I'm typing in this PR. If you feel differently then we can put changes in motion but I feel like there should be a different PR for those. There's a great deal of places where things could be changed or made simpler, but there's only so many changes before things become unmanageable.

ybnd · 2023-05-23T18:42:22Z

beets/util/artresizer.py

@@ -240,10 +259,15 @@ def convert_format(self, source, target, deinterlaced):
            return source

    @property
-    def can_compare(self):
+    def can_compare(self) -> bool:
        return self.version() > (6, 8, 7)


Type checker flags this because it doesn't take into account that the version may be non-existent.
The original signature is a bit too complicated, see my previous comment.

Mypy is not usable at the current stage as an indicator of typing correctness. See next comment.

ybnd · 2023-05-23T18:52:31Z

beets/util/artresizer.py

        return True

-    def write_metadata(self, file, metadata):
+    def write_metadata(self, file: AnyStr, metadata: Mapping):
        assignments = list(chain.from_iterable(


Looks like something about this makes it "untypeable", I think we basically have no choice but to explicitly type it as Any...

beets/util/artresizer.py:344: error: Cannot determine type of "assignments" [has-type]

Doesn't seem like too big of a shortcut to me given that util.command_output is untyped anyway.

If you're using mypy to get these hints, then you should know that I have been working under the understanding that mypy is not really relevant at this stage. It is much more strict and even without this PR, throws a huge number of errors. See here for details.

That MyPy gives this error doesn't mean it's true. It doesn't mean that it is actually Any. Those are the typings that are presented by the codebase and those ones I'm sure of.

Well, I wouldn't say mypy is completely irrelevant, it can definitely provide useful input.

What is certainly the case that having zero mypy issues left is not a requirement for a PR like this to be merged, since that might simply not be achievable without substantial refactoring to resolve issues that mypy reveals.

An example would be the version() problem mentioned in another comment: The refactoring proposed by @ybnd looks like a great idea, but I also agree that we might want to hold it off for another PR in order to avoid blowing up this one too much (although, in this case, I'd be fine either way: the PR is not that huge yet).

As to the particular issue, I'm somewhat surprised that mypy cannot resolve this to at least a List[Any], which would be enough to verify that the list unpacking in the argument list is valid. Maybe explicitly annotating it as List[Any] would be useful? Or, maybe specifying the Mapping in the argument more concretely. I suspect this should be Mapping[str, Any], maybe even Mapping[str, str] (but verifying the latter would require some research into the backend methods).

wisp3rwind

Just had a look through this (notably, I didn't actually run any type checker on it!), and added a few comments.

As @ybnd already pointed out, there are some inconsistencies with the design of the backend classes. Adding an abstract Backend base class seems like a good way to get a hold on them. However, I'd say let's keep that for a follow-up.

wisp3rwind · 2023-06-21T21:09:24Z

beets/util/artresizer.py

@@ -172,7 +182,7 @@ def resize(self, maxwidth, path_in, path_out=None, quality=0,

        return path_out

-    def get_size(self, path_in):
+    def get_size(self, path_in: str) -> Optional[Tuple[int, ...]]:


Suggested change

def get_size(self, path_in: str) -> Optional[Tuple[int, ...]]:

def get_size(self, path_in: bytes) -> Optional[Tuple[int, int]]:

Note that path_in is passed through syspath, i.e. it is a path in beets-internal bytestring-representation.

The output type should match the PIL backend. Probably, we should eventually make the code here validate the IM result completely, including the being exactly length 2 (just catching the IndexError is definitely not enough (if such an error can even happen?)).

wisp3rwind · 2023-06-21T21:14:48Z

beets/util/artresizer.py

@@ -209,7 +223,7 @@ def deinterlace(self, path_in, path_out=None):
            # FIXME: Should probably issue a warning?
            return path_in

-    def get_format(self, filepath):
+    def get_format(self, filepath: AnyStr) -> Optional[bytes]:


Probably out-of scope here; but this code seems to be broken: All backends should really have the same return type, so we should probably be decoding the stdout to a str here.

I actually intend to start my grand project to convert to pathlib today on the core modules, so this should be fixed soon regardless.

wisp3rwind · 2023-06-21T21:18:25Z

beets/util/artresizer.py

+            source: AnyStr,
+            target: AnyStr,
+            deinterlaced: bool,
+    ) -> str:


Suggested change

) -> str:

) -> AnyStr:

given that this returns either of source or target.

On second thought: Probably all of these should be bytes (again, these are paths in beets internal representation, as being evidenced by the syspath call).

wisp3rwind · 2023-06-21T21:28:23Z

beets/util/artresizer.py

        """A boolean indicating whether the resizing method is performed
        locally (i.e., PIL or ImageMagick).
        """
        return self.local_method is not None

-    def get_size(self, path_in):
+    def get_size(self, path_in: AnyStr) -> Union[Tuple[int, int], AnyStr]:


Can you correct this method to return None in the else branch? What's currently going on is just wrong (probably due to my own fault... the git blame is somewhat confusing, not sure what happened. Maybe a botched merge? copy&paste bug?)

I don't think there's a way to conditionally return a single type. I can add it to be Optional though so it recognises that a null value is possible.

Sorry, what I meant is that this should return Optional[Tuple[int, int]], and in the else case return None instead of return path_in.

stale · 2023-12-15T07:04:06Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Serene-Arc added 2 commits January 22, 2023 11:51

Type module

560803e

Add some missing typings

450f352

Make import conditional

8b27ea5

sampsyo added the review-needed label May 21, 2023

ybnd suggested changes May 23, 2023

View reviewed changes

wisp3rwind requested changes Jun 21, 2023

View reviewed changes

stale bot added the stale label Dec 15, 2023

Serene-Arc removed the stale label Dec 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add typings to the art resizer module #4649

Add typings to the art resizer module #4649

Serene-Arc commented Jan 22, 2023

wisp3rwind commented Feb 12, 2023

ybnd left a comment

ybnd May 23, 2023

Serene-Arc Jun 2, 2023

wisp3rwind Jun 21, 2023

ybnd May 23, 2023

Serene-Arc Jun 2, 2023

ybnd May 23, 2023

Serene-Arc Jun 2, 2023

ybnd May 23, 2023 •

edited

Loading

Serene-Arc Jun 2, 2023

wisp3rwind Jun 21, 2023

wisp3rwind left a comment

wisp3rwind Jun 21, 2023

wisp3rwind Jun 21, 2023

Serene-Arc Jun 22, 2023

wisp3rwind Jun 21, 2023

wisp3rwind Jun 21, 2023

Serene-Arc Jun 22, 2023

wisp3rwind Jun 22, 2023

stale bot commented Dec 15, 2023

	def get_size(self, path_in: str) -> Optional[Tuple[int, ...]]:
	def get_size(self, path_in: bytes) -> Optional[Tuple[int, int]]:

Add typings to the art resizer module #4649

Are you sure you want to change the base?

Add typings to the art resizer module #4649

Conversation

Serene-Arc commented Jan 22, 2023

wisp3rwind commented Feb 12, 2023

ybnd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ybnd May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wisp3rwind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Dec 15, 2023

ybnd May 23, 2023 •

edited

Loading