New approach to lazy loading of pygame submodules (surfarray, sndarray)
#3249
Conversation
docs/reST/ref/sndarray.rst
Outdated
| @@ -23,6 +23,8 @@ Each sample is an 8-bit or 16-bit integer, depending on the data format. A | |||
| stereo sound file has two values per sample, while a mono sound file only has | |||
| one. | |||
|
|
|||
| .. versionchanged:: 2.5.3 sndarray module is lazily loaded to avoid loading NumPy needlessly | |||
There was a problem hiding this comment.
This still doesn't explain why the action is actually being taken.
Why is it bad to load NumPy?
|
@damusss You cast some doubt about this PR working with PyInstaller on discord but did not elaborate. If there's a problem here can you explain what you did please. |
🫡 I tested both normal pygame-ce and this branch, and it looks like both imports are successful. when I got that error I must have been using an incomplete version of the pull request |
|
Looks like this change has uncovered a pylint bug. Checking it locally it doesn't happen. Looks like a recurrence of this bug: Which the last commenter said only happened on CI. Maybe there is some multithreaded shenanigans happening. |
|
Both are with numpy installed. (Without numpy or after loading numpy, times are the same.) Simple import First attribute access after importing pygame (note: if accessing just |
|
These benchmarks makes sense, thanks for working on it. You mention "first attribute access" I wonder how the performance of subsequent attribute accesses are affected (if it is) |
|
After the first attribute access, I'm fairly certain both have the same access times. (There is no module |
aatle
added
the
Performance
|
@coderabbitai full review please |
|
@aatle I'll conduct a full review of this PR implementing the new lazy loading approach for pygame submodules. ✅ Actions performedFull review triggered. |
WalkthroughAdds lazy loading for pygame.surfarray and pygame.sndarray via a new lazy_import helper in src_py/init.py, defers NumPy import until first access, preserves MissingModule behavior when NumPy is absent, and documents the change in sndarray and surfarray reST files. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Python
participant pygame as pygame.__init__
participant NumPy
participant surfarray
participant sndarray
participant Packager
User->>Python: import pygame
Python->>pygame: execute initialization
pygame->>NumPy: check availability (find_spec)
alt NumPy present
pygame->>pygame: set surfarray = lazy_import("surfarray")
pygame->>pygame: set sndarray = lazy_import("sndarray")
else NumPy missing
pygame->>pygame: set surfarray = MissingModule
pygame->>pygame: set sndarray = MissingModule
end
Packager->>pygame: scan source / call packager_imports()
pygame->>Packager: contains explicit imports for surfarray/sndarray
User->>pygame: access pygame.surfarray or import pygame.surfarray
pygame->>surfarray: load module on first attribute access (triggers NumPy import inside module if needed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes(No out-of-scope functional code changes detected; documentation additions and init behavior align with the linked issue objectives.)
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
docs/reST/ref/sndarray.rst (1)
26-27: LGTM; consider adding a brief rationale to preempt “why avoid loading NumPy?”The directive is correct. To address prior review feedback asking “why is it bad to load NumPy?”, add a short rationale to clarify the benefit.
.. versionchanged:: 2.5.6 sndarray module is lazily loaded to avoid an expensive NumPy import when unnecessary + Rationale: reduces pygame import time and memory footprint for apps that don't use sndarray.
🧹 Nitpick comments (1)
src_py/__init__.py (1)
328-338: Optional: deduplicate the repeated optional-module initialization pattern.Both blocks have the same structure (probe numpy -> check dependency -> MissingModule or lazy import). Consider factoring into a small helper to reduce duplication and keep future changes consistent.
Example helper (outside the shown ranges):
def _init_optional_lazy(name: str, deps: tuple[str, ...]) -> None: try: if numpy_missing: import numpy # for MissingModule.reason for dep in deps: __import__("pygame." + dep) except (ImportError, OSError): globals()[name] = MissingModule(name, urgent=0) else: globals()[name] = lazy_import(name) # Usage: # _init_optional_lazy("surfarray", ("pixelcopy",)) # _init_optional_lazy("sndarray", ("mixer",))Also applies to: 339-349
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/reST/ref/sndarray.rst(1 hunks)docs/reST/ref/surfarray.rst(1 hunks)src_py/__init__.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src_py/__init__.py (1)
src_c/pixelcopy.c (1)
pixelcopy(1250-1275)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src_py/__init__.py (3)
311-325: Hardened for missing spec/loader — matches prior feedbackThe added guard for spec is None or spec.loader is None prevents AttributeError when modules are stripped. This addresses the earlier review concern.
349-357: Same pattern as surfarray; consider shared helper for maintainabilityMirrors the surfarray approach with pygame.mixer as the dependency. Consider the shared helper suggested above to reduce duplication and keep behavior in sync.
421-424: Make packager_imports resilient if numpy/deps are missing in packaging environmentsSome tooling may execute or partially evaluate this function; unguarded imports of lazily-available modules can raise. Wrapping avoids hard failures while remaining visible to static scanners.
Apply:
- # lazy imports - import pygame.surfarray - import pygame.sndarray + # lazy imports (guarded to avoid failures if executed by tooling) + try: + import pygame.surfarray + except Exception: + pass + try: + import pygame.sndarray + except Exception: + pass
🧹 Nitpick comments (2)
src_py/__init__.py (2)
311-331: LazyLoader pattern is correctly implemented; consider registering a stub for stripped builds (optional)The LazyLoader flow matches the docs and uses the fully-qualified fullname key in sys.modules. Nice. For stripped builds (spec is None), you currently return a MissingModule but do not register it in sys.modules. If you want import pygame.surfarray to succeed with a stub in those builds (instead of raising ModuleNotFoundError), optionally register the stub:
Apply if you want import pygame. to resolve to the stub; otherwise, keep current behavior to continue raising on stripped builds.
def lazy_import(name): @@ - if spec is None or spec.loader is None: - return MissingModule(name, urgent=0) + if spec is None or spec.loader is None: + m = MissingModule(name, urgent=0) + # Optionally register to let `import pygame.<name>` succeed with a stub + # Comment out the next line if you prefer a ModuleNotFoundError on stripped builds. + sys.modules[fullname] = m + return mPlease confirm the intended behavior for stripped builds:
- Keep raising on import pygame.surfarray / pygame.sndarray (status quo)?
- Or resolve to a MissingModule stub across all import forms?
338-347: Preflight dependency check + reason capture looks good; minor DRY opportunity
- Intentionally importing numpy in the numpy_missing branch to capture an informative error message for MissingModule.reason is a pragmatic solution and avoids loading numpy otherwise.
- Checking pygame.pixelcopy ensures surfarray won’t be lazily installed if a hard dependency is missing.
Optional: reduce duplication between surfarray/sndarray blocks by extracting a small helper.
Example helper (outside current ranges, for illustration):
def _maybe_lazy_import(submod, dep_module): try: if numpy_missing: import numpy # force error for informative reason __import__(dep_module) except (ImportError, OSError): return MissingModule(submod, urgent=0) else: return lazy_import(submod) # usage: surfarray = _maybe_lazy_import("surfarray", "pygame.pixelcopy")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
docs/reST/ref/surfarray.rst(1 hunks)src_py/__init__.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/reST/ref/surfarray.rst
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: arm64
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: i686
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: AMD64
- GitHub Check: x86
- GitHub Check: dev-check
🔇 Additional comments (4)
src_py/__init__.py (4)
76-83: Good: MissingModule now captures exception info for clearer errorsCapturing the exception type and message via sys.exc_info() and surfacing them in reason improves diagnostics and preserves prior semantics when constructed inside except blocks.
334-336: Lightweight NumPy availability check is appropriateUsing find_spec("numpy") avoids side effects and import-time cost. Good trade-off for the lazy-load gate.
359-359: Namespace cleanup is sensibleDeleting helper symbols keeps the pygame namespace tidy after initialization.
410-414: Clearer packager_imports docstring — good context for toolingCalling out that the function is never executed and serves static scanners is helpful to avoid confusion.
A new approach to lazy loading pygame submodules after #3232 had a problem with slower attribute accesses (
__getattr__).Implementation explanation:
The implementation is twofold, where the second part has many alternatives.
First, pygame checks if
numpyexists.Then, each submodule is loaded if numpy exists and the submodule's other pygame module dependencies (e.g.
mixer) load successfully. If numpy is missing or a module dependency fails, then the submodule becomesMissingModule. It is necessary to get an error to use its message inMissingModule.reason, sotry-exceptis used.If successful, the submodule is created and put into
sys.modules, using the documentedLazyLoaderimplementation, relying on import machinery.This makes it so that the submodule exists, but its contents are not executed until the first attribute or method access on the submodule.
It does this by changing the module class temporarily to
_LazyModule; after the module is actually loaded, the module class is set back to the normal module type.Potential differences from current behavior:
The initial checks are not completely perfect. It is assumed that if
numpyis importable, it will not raise anImportErrornorOSError.Obviously,
numpynot being loaded by pygame automatically is an intended difference. Instead, it is loaded upon the first attribute access of either submodule. I do not expect this to cause any lag spikes in-game: the user probably imports numpy themselves, and it will also load if directly importing one of the submodule functions, or using one during game loading.After a submodule is loaded successfully, there should not be any noticeable difference from current behavior. The module class is the same, the original loader is put back, the module code was not changed.
https://docs.python.org/3/library/importlib.html#importlib.util.LazyLoader
There are no expected errors during postponed loading of these submodules, as they should have all been handled at the start.
Lazy loading of numpy has great potential benefits, so it's worth the extra complexity.
Closes #3232
Summary by CodeRabbit
Refactor
Documentation