Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement lazy loading of pygame submodules (surfarray, sndarray) #3232

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

aatle
Copy link
Contributor

@aatle aatle commented Nov 21, 2024

Motivation:
Previously, I discovered via profiling that numpy was being imported or loaded in my pygame program, when there were no mandatory runtime dependencies on it, though I had it installed.
The pygame submodules surfarray and sndarray both attempt to import numpy if possible, and pygame always tries to load these submodules. (If numpy is not available, they are replaced by MissingModule.)

When running import pygame, numpy (if available) loading takes up at least 40% of the time, around 100 milliseconds depending on the hardware. However, the surfarray and sndarray modules are not used in many pygame programs, and some users install numpy globally, or, like me, have dev dependencies that use numpy.

Implementation:
To fix this, numpy shouldn't be loaded unless it is actually needed by the user. This requires lazy loading/importing. Obviously, the implementation should be backwards-compatible.

Considered lazy import implementations

Five ways to implement lazy loading that I know of:

  1. Don't import submodule in the parent package itself
  2. Put all expensive imports inside of functions
  3. Use importlib.util.LazyLoader
  4. Use a custom ModuleType subclass, as in https://pypi.org/project/lazy-imports/
  5. (Chosen) Use the module __getattr__ feature (python 3.7+), similar to as in https://snarky.ca/lazy-importing-in-python-3-7/

Only the fifth is suitable because of backwards compatibility. It also happens to be one of the simplest.
Problems with 1 - 4:

  1. Cannot implement the existing MissingModule behavior (when numpy is unavailable)
  2. Too much effort, massive changes to code of the submodules. Does not preserve MissingModule behavior
  3. Cannot implement MissingModule upon import failure
  4. Too complicated. Cannot implement MissingModule behavior

The pygame submodules surfarray and sndarray will be lazily imported, deferring execution/loading of them to when the user themselves access it (if they do), e.g. pygame.surfarray.array2d or import pygame.sndarray.
If they do use it, numpy will usually not be loaded at an inconvenient time (to cause framedrops) unless the first use (including imports) is of the form pygame.surfarray.___ inside a function (easy user fix).

Backwards compatibility / Testing:

All combinations of these factors must be compatible
  • numpy is or is not installed
  • sndarray, surfarray is or is not accessed by user
  • If accessed, the access method used:
    • import pygame.surfarray
    • from pygame import surfarray
    • pygame.surfarray.array2d
    • from pygame.surfarray import array2d

In addition:

  • Accessing undefined attribute on pygame module (same error message)
  • PyInstaller and such must still work

I did not include test cases in this PR, though I think the implementation is probably compatible.

Notes:
Since MissingModule class is deleted at the end of __init__.py and unavailable to __getattr__, there is a new private alias _MissingModule in the pygame namespace.
I don't like the MissingModule behavior that much because half of the access forms bypass it.
If this PR is merged, then more optional pygame submodules may be lazily imported in the future.

@aatle aatle requested a review from a team as a code owner November 21, 2024 02:21
# lastly, the "optional" pygame modules

_MissingModule = MissingModule
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would make sense to store this as a local variable on __getattr__

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MissingModule class is deleted at the end of the module, and __getattr__ still needs it, so it must be stored somewhere (not as local variable).
A few options:

  • Private alias of MissingModule
  • Move MissingModule to another file
  • Bind MissingModule as a default keyword argument in __getattr__

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an idea:

>>> a = 36
>>>
>>> def func():
...     print(func.b)
...
>>>
>>> func.b = a
>>> del a
>>> func()
36

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I suggested that before but ultimately opted for a private alias because function attributes are kind of weird and dynamic. From my reasoning, it would've look strange and I would rather avoid these rare python features.
If it is necessary to avoid exposing _MissingModule in the pygame namespace, then sure, but I think the current approach is okay.

@Starbuck5
Copy link
Member

There's a block in here called def packager_imports(): for showing packagers imports that might be missed. I'm not sure it's necessary for this situation but adding explicit surfarray/sndarray imports there would make me feel more confident in this solution.

Also I think this could be worth a small versionchanged or note in the docs.

@Starbuck5
Copy link
Member

Also, for reviewer reference, the command to test this is python -X importtime -c "import pygame"

@aatle
Copy link
Contributor Author

aatle commented Nov 30, 2024

@damusss uncovered a problem
Normal attribute accesses to the pygame module are affected by the presence of __getattr__ too, leading to slower attribute accesses.
This most likely means a different implementation will have to be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants