Possible removal of 'type juggling' of references (mypy related)

[matt-land]

I am working on #56, and having is some difficulty. I am using mypy, which was used in the original issue to identity files with untyped functions.
Mypy has exposed a code smell in this repo (type juggling- example below), which was originally intentionally, for the sake of saving memory in CircuitPython.

class Palette:
    """ ... implemented in Adafruit_Blinka_Display.displayio """
palette = Palette.init  # palette's type is a Callable (we may or not need this object later, so lets defer and pass down a reference to a Callable)
... its later, in a different file ...
def load(..., palette = None, ...):
    ....
    palette = palette(*args) # palette's type is now Palette instance
    palette.make_transparent()  # mypy is now really angry, because a Callable shouldn't have any methods 

There is a certain elegance to the way it was done: reuse the reference to the Callable, possibly allowing deallocation of the reference to the looked up function. But this is counter to what Mypy expects, because its a behavior commonly associated with having a bug in your code: Developer thinks they have a foo, but they accidentally overwrote it somewhere with variable name reuse.

So discuss the balance between "we want working type annotations, so the code is understandable and maintainable" and "we need to save memory through tricks, and are ok with here be dragons". Scott wrote & I refactored a lot of the original code 3 years ago. Now looking back, It took me ~3 hours to determine what was going on, meaning there is a high barrier to entry for contributors. I'll be in the camp for "slay the dragons", and stop reusing variable names.

I know there are inline mypy decorators that can be dropped throughout the code to help get closer to mypy 'passing' by explicitly indicating when it happens, but its going to take a lot of them (every time the type changes) and be precarious to maintain. Inline decorators are intended to be used in exceptional cases, but there are many in the repo. So not a real option.

So:

• compromise on types: add best effort types, mypy never passes, so automated tooling can't be added to maintain types in the repo. Memory footprint is maintained, but the repo is still difficult to contribute to, and teaches anti-patterns.
• compromise on memory: a future release of imageload removes type juggling at the cost of slightly higher memory usage, and type checking can be added as a commit hook. Worst case is some memory constrained board that worked with a past version of the library now runs out of memory.

[matt-land]

Example from code, there are many:
https://github.com/adafruit/Adafruit_CircuitPython_ImageLoad/blob/main/adafruit_imageload/pnm/ppm_binary.py#L37-L40

[kattni]

@FoamyGuy @dhalbert @tekktrik @jepler I want to open a discussion about this, so I asked @matt-land to file an issue based on what he found.

Let's discuss!

[dhalbert]

stop reusing variable names

I would agree with this. If variables were typed/annotated, we wouldn't be doing this at all. Since reference counting is not used, it doesn't matter a lot anyway; it just takes some extra dict slots.

[tekktrik]

Does mypy get less angry if the palette argument is typed as Callable[[int], Palette]? Is the actual instance ever saved, or can the palette argument ever be anything other than Callable? Just trying to understand if there's an issue with trying to type annotate it.

As for type juggling, I agree with you and @dhalbert. I didn't realize reference counting isn't used! Would strategic use of gc.collect() help in this case?

[dhalbert]

Would strategic use of gc.collect() help in this case?

Unless there's something potentially really large or fragmenting to get rid of, we can just wait until a regular gc.

[FoamyGuy]

I do think it's a good discussion to have. I would generally be in favor of making the code more understandable (and thus easier to contribute to) at the potential expense of RAM unless it causes it to become completely unusable on the "smallest" devices. Maybe if more if a more maintainable version of this library does push us over that threshold it would be worth "freezing" the current version as is for use on those smaller devices, but then bumping a major rev version and update it moving forward with the more maintainable version knowing it may not work on those smallest environments?

I don't have much experience with this codebase but I do know of another instance of stuff done in a strange way that ends up allowing it to still work in certain environments at the cost of the weirder code. The way negative_height_check was refactored into it's own file allows this library to work in environments without support for long_int. This is only tangentially related to the memory usage discussion, but similarly we had to add ignores to appease pylint.

I'lll take a closer look into the code in this library. Off the top of my head I'm curious about why Palette would ever be a Callable instead of a Palette object or maybe PixelShader object. I don't recall seeing any usage where a function was set as a Palette. Possibly I'm misunderstanding this bit though. Hopefully I'll have a better idea once I look further into the code.

[matt-land]

Wow, your responses are fast!

As to palette reference being callable vs a Palette (and Bitmap):

• they are assigned as Callable's here https://github.com/adafruit/Adafruit_CircuitPython_ImageLoad/blob/main/adafruit_imageload/__init__.py#L34-L37
• various methods are used to detect the file type (headers) and once determined, a matching strategy's source code is late imported. This is another micropython optimization- only load the source for the kind of file being loaded, after the fact.
• the callables are passed to the strategy to be instantiated, to be instantiated only if needed, and with parameters that make sense for that strategy. A two color bitmap gets a Palette with 2 colors, etc, A bitmap is allocated based on the resolution of the file being read. Sometimes Palette isn't needed so its constructor is never called.
  I have a WIP PR here, which shows callables being passed down to each strategy, and concrete Bitmap and Palette objects being (optionally) being returned:
  WIP adding types for #56 Missing type annotations matt-land/Adafruit_CircuitPython_ImageLoad#1

@tekktrik I can try and re-type a reference with allocations, and it seems to make my IDE happy, but Mypy has some other ambiguous scopes, and per reading their docs, bind on the indentation level so they often don't work in cases like this, because the re-binding of the type is nested inside an if:
https://github.com/matt-land/Adafruit_CircuitPython_ImageLoad/pull/1/files#diff-332fafb6cd78908a84a8a46ea00dbe8da297a5ea2f75ce3b66d0b8119dca641fR44-R46

[matt-land]

Working on this at pyconus 2023

[FoamyGuy]

resolved by #66