implement lazy imports #2584
implement lazy imports #2584
Conversation
|
There are some flake8 errors (find "pre-commit checks" below and click "Details" on the right), mostly like this: These can be suppressed by adding a noqa marker: However, I'm not sure whether
|
|
The core unit tests (scroll through the checks below and click "Details") are now failing with this error: This is a problem with the tests, so it isn't your fault, but you'll still have to fix it before the PR can be merged. The problem is that the tests assume that all the This can be fixed in that file by changing To run the tests on your own machine, install Toga into your environment and then follow these instructions. |
all the tests pass on my end. |
Does this include both the core API tests and the testbed tests? Since the testbed tests are failing on every platform (except iOS) in CI, it suggests you're only running the core API tests locally. |
There was a problem hiding this comment.
I've cleaned up some import usage and the changenote; but the bigger question is about the changes to the Cocoa factory module.
Lazy loading in the backends there would definitely make sense... but why has only the cocoa backend been updated in this way? (And, you'll note, it's then the backend that fails testing, because it's lazily loading the core classes, not the backend classes).
Each backend can do this independently, so they don't all need to be done in the same PR. I've removed the "fixes" keyword from the top comment to reflect this. |
Well... sure... but the logic for each backend will be identical, so I'm not sure why we wouldn't duplicate across all the backends (or, at the very least, the desktop, mobile and dummy backends). On top of that, via Discord we found out that the one platform the reporter doesn't have access to is macOS, so it seems a weird choice to start there. |
Update factory.py Cam Lock sorted lazy mobile lazy mobiles final final stretch final stretch pre-commit coverage fix up the fixup of the fixups fixup the fixups fixup precommit fixup fixup not_impl fn hiccup precommit
Update factory.py Cam Lock sorted lazy mobile lazy mobiles final final stretch final stretch pre-commit coverage fix up the fixup of the fixups fixup the fixups fixup precommit fixup fixup not_impl fn hiccup precommit
There was a problem hiding this comment.
A couple of minor stylistic tweaks inline; however, the bigger issue for me is the code duplication. The logic in each backend's factory module is now identical, except for the name of the module, and the list of widgets that are in the mapping; the former can be easily parameterised, and the latter is detail that can be determined programmatically by trying to import a name and succeding/failing.
That leads me to question whether the factory modules are needed at all.
The Factory module was needed previously because it included literal imports; but if we're replacing all those literal imports with programmatic ones, it seems to me that we should be replacing all the factory.py modules with a single Factory class that is instantiated in toga.core.platform, whose __getattr__ implementation does the dynamic submodule lookup in the way it's being done here for each module. The lookup mechanism will be almost identical - just parameterised on the module name, and with an extra layer of ImportError handling.
| ) from None | ||
| else: | ||
| module = importlib.import_module(module_name) | ||
| value = getattr(module, name) if has_dot else module |
There was a problem hiding this comment.
This logic works, but it's a bit convoluted - it involves evaluating has_dot twice. You can get the same effect in less lines of code using rsplit() and iteration:
base_name, *extra = module_name.rsplit('.', 1)
value = importlib.import_module(base_name)
for name in extra:
value = getattr(value, name)
Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
That's a good idea, but I think it can be made even simpler, at the cost of changing all the code that uses the factory. The factory takes a perfectly usable multi-level namespace from the backend package, and creates a new single-level namespace out of it for no significant benefit. What if we instead make the interface layer access the backend module structure directly? For example, where we currently write: We would instead have: Which on macOS would translate into an import of That way, the names of the backend modules and classes would be distributed throughout the interface layer in the places each of them is used, rather than centralized in one list. |
|
Places which use the same name multiple times can be refactored slightly. For example, in |
I guess that's true: there's no reason to use a single "Factory" namespace, as long as we can clearly specify that path inside the backend module that will contain the class we want to use. I guess the only question I'd have is whether we include the class name in the import calls (e.g., |
Yes, that would be a bit cleaner. And in most cases, each interface module only uses one backend module, so we could adopt the pattern of calling it and then
The only other argument of |
Agreed this would be nice, provided there's no circular import or other dependency issues lurking.
|
|
In that case, how about we reduce this PR to just doing lazy imports in the core, and record a separate issue for the factory change ideas? |
I'd be OK with that - but either way, this PR will need to be modified (either adding the new imports, or removing the dynamic import implementation on the platform backends being used here); which way we go depends on @KRRT7's interest level. |
Good point: there are a few places where the backend needs to access things from the frontend at module level, like |
let's do this, and let's open the factory change ideas and once you guys have come to a decision on what to do, i'll give it a look and discuss how best to implement it. |
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)
adds a lazy import mechanism using a module-level getattr function. This applies both to:
PR Checklist: