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 in toga_core #2686

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

Conversation

KRRT7
Copy link

@KRRT7 KRRT7 commented Jul 1, 2024

adds a lazy import mechanism using a module-level getattr function. This applies to:

  1. toga-core (in toga/init.py)
    Only

PR Checklist:

  • [ X] All new features have been tested
  • [ X] All new features have been documented
  • [X ] I have read the CONTRIBUTING.md file
  • [ X] I will abide by the code of conduct

@KRRT7
Copy link
Author

KRRT7 commented Jul 1, 2024

this comes behind #2584 , it was very messy and would require lots of reverts, please see there for history and revelance.

@mhsmith mhsmith mentioned this pull request Jul 1, 2024
4 tasks
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

The core of this looks good; a couple of minor cleanups flagged inline.

core/src/toga/__init__.py Outdated Show resolved Hide resolved
core/src/toga/__init__.py Outdated Show resolved Hide resolved
core/src/toga/__init__.py Outdated Show resolved Hide resolved
core/src/toga/__init__.py Show resolved Hide resolved
KRRT7 added 4 commits July 3, 2024 12:03
This list should be in alphabetical order by module and then by name, as in toga/__init__.py.
beeware#2584 (comment)
including  preserving the historical sort order (i.e., sorted by the full import path of the original module), with interleaved comments highlighting that ordering.
beeware#2686 (comment)
@KRRT7
Copy link
Author

KRRT7 commented Jul 8, 2024

The core of this looks good; a couple of minor cleanups flagged inline.

Thanks, the issues were resolved

@mhsmith
Copy link
Member

mhsmith commented Jul 8, 2024

This all looks good to me, but even performance improvements should have a test where possible. Since the tests are run in alphabetical order, the app tests are run first, so we could put a test in there which does the following:

  • Pick a widget module that isn't used elsewhere in the app tests, e.g. toga.widgets.button.
  • Assert that the module doesn't exist in sys.modules.
  • Access toga.Button.
  • Assert that the module now exists in sys.modules, and that toga.Button is toga.widgets.button.Button.

@freakboy3742
Copy link
Member

freakboy3742 commented Jul 11, 2024

I'd be a little wary of adding a test that depends on execution order - while default invocation of pytest will execute in alphabetical order, there's no guarantee tests will be executed in that order (especially if they're manually specified).

An explicit test that a widget such as Button has been imported correctly (whether by the test, or by running a Button test previously) is definitely called for - but I don't think the "from a clean state" requirement (i.e., that sys.modules was clean beforehand) is needed.

@mhsmith
Copy link
Member

mhsmith commented Jul 12, 2024

There should still be a test verifying that a clean import of toga doesn't import all the widgets. Otherwise, if that somehow started happening again in the future, we might not notice for a long time.

The test itself could create the clean slate by removing toga and toga.widgets.button from sys.modules, and restoring them afterwards. Then it wouldn't matter which order it ran in, and it could be placed somewhere more appropriate than the app directory.

@freakboy3742
Copy link
Member

The test itself could create the clean slate by removing toga and toga.widgets.button from sys.modules, and restoring them afterwards. Then it wouldn't matter which order it ran in, and it could be placed somewhere more appropriate than the app directory.

True - although there's also the implementation cache of globals() that needs to be cleared.

That said - looking at the current implementation, the value is being set in globals(), but that cached value is never being used... as written, every import will fire the import machinery, and the value in globals() will never be used.

@mhsmith
Copy link
Member

mhsmith commented Jul 15, 2024

True - although there's also the implementation cache of globals() that needs to be cleared.

Every module object has its own globals, so a freshly-imported toga module will have nothing cached.

That said - looking at the current implementation, the value is being set in globals(), but that cached value is never being used

See "customizing module attribute access" – adding the value to globals allows it to be found "through the normal lookup", so __getattr__ will not be called with that name again.

@freakboy3742
Copy link
Member

See "customizing module attribute access" – adding the value to globals allows it to be found "through the normal lookup", so __getattr__ will not be called with that name again.

Huh - my mental model has __getattr__ responsible for all module attribute lookup if it existed. If it's only used as a supplement to what is found in globals(), then I agree this isn't an issue.

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

Successfully merging this pull request may close these issues.

3 participants