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

Concise and static Python imports #2135

Merged
merged 27 commits into from
Nov 12, 2021
Merged

Conversation

Helveg
Copy link
Contributor

@Helveg Helveg commented Aug 25, 2021

Removed all forms of dynamic module importing and from X import *. If the need for dynamic modules arises again they can be defined as a lazy loaded module such as spatial with some extra indications for the user when missing optional modules are accessed:

>>> nest.optional.feature()
ImportError: This functionality requires the optional `optional` module. See <nest-docs> to install it.

PR summary

  • Replaced ast/compile based dynamic importing by importlib.
    • ast/compile sidesteps import machinery.
    • Added mechanism for optional modules _lazy_module_property("feature", optional=True, optional_hint="Please install feature.")
  • Removed hl_api
  • Replaced from X import * calls with explicit imports.
  • Simplified test imports/discovery, removed gratuitous test helper file.

@Helveg

This comment has been minimized.

* Caused code duplication (__all__)
* Is a maintenance burden
* Namespace pollution
@Helveg
Copy link
Contributor Author

Helveg commented Sep 13, 2021

@jougs I hope you don't mind I went ahead and removed hl_api.py prematurely, the code in it would only confuse developers. I checked the codebase and it wasn't used anywhere except for a couple of times as nest.hl_api.serializable, I think also #2157 changes those to just nest.serializable.

The change could have affected users that do funky things with deep imports, but I just added nest as nest.hl_api so they can safely continue importing from there until we clean that up in 4.0. I'm actually all for removing it altogether because I doubt many people actually use these deep imports.

@Helveg Helveg marked this pull request as ready for review September 13, 2021 16:09
@jougs
Copy link
Contributor

jougs commented Sep 13, 2021

No worries! I'm always in favor of cleaning things up and not adding backwards-compatibility-cruft.

As this PR won't make it into 3.1, I suggest, you create another small PR, adding a sentence to the release notes that the hl_api namespace will go away with 3.2.

@Helveg
Copy link
Contributor Author

Helveg commented Sep 13, 2021

(side note on #2157 to @jougs and in general also for @heplesser and others related to the modules)

All of our problems with the reference to the old nest module seem to be disguised circular import errors that due to a bug in Python makes it pick up the unreplaced module object that it compiled and executed in order to even get to the point in __init__.py where we try to replace the sys.modules object, but then because of circular imports it silently doesn't finish making that object and the old one probably dangles around somewhere..

I think the codebase should be thoroughly vetted for code that gets executed at import time and also the nest submodules should not import nest itself at import time, but the sibling modules that actually provide the logic they're looking for. i.e. from .spatial import thing_in_spatial not from . import thing_in_spatial. I haven't checked how often that occurs but it relies on the fact that everything is shortcut unto nest on import. This is only done for user convenience and developers should never use these shortcuts internally as they cause circular imports when nest is loading.

@stinebuu stinebuu added this to In progress in PyNEST via automation Sep 27, 2021
@stinebuu stinebuu moved this from In progress to Review in PyNEST Sep 27, 2021
@stinebuu stinebuu added I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. labels Sep 27, 2021
@stinebuu stinebuu requested a review from jougs September 29, 2021 12:12
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

Other than my few comments (that were likely triggered by my ignorance about how pytest works) I'm fine with this. Thanks for the nice contribution :-)

pynest/nest/lib/hl_api_simulation.py Show resolved Hide resolved
testsuite/pytests/test_sp/__init__.py Outdated Show resolved Hide resolved
testsuite/pytests/test_spatial/__init__.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

Thanks for the nice contribution and the fruitful discussion.

Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

@Helveg Looks generally good to me, just two small suggestions.

pynest/nest/__init__.py Outdated Show resolved Hide resolved
pynest/nest/__init__.py Outdated Show resolved Hide resolved
Co-authored-by: Hans Ekkehard Plesser <hans.ekkehard.plesser@nmbu.no>
@Helveg Helveg requested a review from heplesser November 8, 2021 09:39
@jougs jougs removed the request for review from clinssen November 12, 2021 08:41
@abigailm abigailm merged commit b6ddf7e into nest:master Nov 12, 2021
PyNEST automation moved this from Review to Done Nov 12, 2021
@Helveg Helveg deleted the static-py-imports branch November 12, 2021 10:11
@jougs jougs linked an issue Mar 7, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation.
Projects
PyNEST
  
Done
Development

Successfully merging this pull request may close these issues.

Remove blanket and dynamic Python module importing
6 participants