-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Module imported twice under import-mode=importlib #10811
Comments
In pmxbot/pmxbot@3adc54c, I've managed to pare down the project to a bare minimum reproducer. The issue only happens when This issue may be related to (or same as) #10341. I think you'll agree this is pretty basic behavior that should be supported. I'm not even aware of a good workaround. |
Hey @jaraco, thanks for the reproducer! I found the problem, will open a PR shortly. |
The initial implementation (in pytest-dev#7246) introduced the `importlib` mode, which never added the imported module to `sys.modules`, so it included a test to ensure calling `import_path` twice would yield different modules. That proved problematic, so we started adding the imported module to `sys.modules` in pytest-dev#7870, but failed to realize that given we are now changing `sys.modules`, we might as well avoid importing it more than once. Then pytest-dev#10088 came along, passing `importlib` also when importing application modules (as opposed to only test modules before), which caused problems due to imports having side-effects and the expectation being that they are imported only once. With this PR, `import_path` returns the module immediately if already in `sys.modules`. Fix pytest-dev#10811, pytest-dev#10341
The initial implementation (in pytest-dev#7246) introduced the `importlib` mode, which never added the imported module to `sys.modules`, so it included a test to ensure calling `import_path` twice would yield different modules. Not adding modules to `sys.modules` proved problematic, so we began to add the imported module to `sys.modules` in pytest-dev#7870, but failed to realize that given we are now changing `sys.modules`, we might as well avoid importing it more than once. Then pytest-dev#10088 came along, passing `importlib` also when importing application modules (as opposed to only test modules before), which caused problems due to imports having side-effects and the expectation being that they are imported only once. With this PR, `import_path` returns the module immediately if already in `sys.modules`. Fix pytest-dev#10811, pytest-dev#10341
Btw I noticed that if you call |
Yes, but that also effectively disables doctests.
Neat! I'll test it against the full pmxbot branch that's switching to a native namespace package implementation. |
The initial implementation (in pytest-dev#7246) introduced the `importlib` mode, which never added the imported module to `sys.modules`, so it included a test to ensure calling `import_path` twice would yield different modules. Not adding modules to `sys.modules` proved problematic, so we began to add the imported module to `sys.modules` in pytest-dev#7870, but failed to realize that given we are now changing `sys.modules`, we might as well avoid importing it more than once. Then pytest-dev#10088 came along, passing `importlib` also when importing application modules (as opposed to only test modules before), which caused problems due to imports having side-effects and the expectation being that they are imported only once. With this PR, `import_path` returns the module immediately if already in `sys.modules`. Fix pytest-dev#10811, pytest-dev#10341
The initial implementation (in pytest-dev#7246) introduced the `importlib` mode, which never added the imported module to `sys.modules`, so it included a test to ensure calling `import_path` twice would yield different modules. Not adding modules to `sys.modules` proved problematic, so we began to add the imported module to `sys.modules` in pytest-dev#7870, but failed to realize that given we are now changing `sys.modules`, we might as well avoid importing it more than once. Then pytest-dev#10088 came along, passing `importlib` also when importing application modules (as opposed to only test modules before), which caused problems due to imports having side-effects and the expectation being that they are imported only once. With this PR, `import_path` returns the module immediately if already in `sys.modules`. Fix pytest-dev#10811, pytest-dev#10341
The initial implementation (in #7246) introduced the `importlib` mode, which never added the imported module to `sys.modules`, so it included a test to ensure calling `import_path` twice would yield different modules. Not adding modules to `sys.modules` proved problematic, so we began to add the imported module to `sys.modules` in #7870, but failed to realize that given we are now changing `sys.modules`, we might as well avoid importing it more than once. Then #10088 came along, passing `importlib` also when importing application modules (as opposed to only test modules before), which caused problems due to imports having side-effects and the expectation being that they are imported only once. With this PR, `import_path` returns the module immediately if already in `sys.modules`. Fix #10811 Fix #10341
This PR contains the following updates: | Package | Update | Change | |---|---|---| | [pytest](https://docs.pytest.org/en/latest/) ([source](https://github.com/pytest-dev/pytest), [changelog](https://docs.pytest.org/en/stable/changelog.html)) | patch | `==7.4.0` -> `==7.4.2` | --- ### Release Notes <details> <summary>pytest-dev/pytest (pytest)</summary> ### [`v7.4.2`](https://github.com/pytest-dev/pytest/releases/tag/7.4.2): pytest 7.4.2 (2023-09-07) [Compare Source](pytest-dev/pytest@7.4.1...7.4.2) ### Bug Fixes - [#​11237](pytest-dev/pytest#11237): Fix doctest collection of `functools.cached_property` objects. - [#​11306](pytest-dev/pytest#11306): Fixed bug using `--importmode=importlib` which would cause package `__init__.py` files to be imported more than once in some cases. - [#​11367](pytest-dev/pytest#11367): Fixed bug where `user_properties` where not being saved in the JUnit XML file if a fixture failed during teardown. - [#​11394](pytest-dev/pytest#11394): Fixed crash when parsing long command line arguments that might be interpreted as files. ### Improved Documentation - [#​11391](pytest-dev/pytest#11391): Improved disclaimer on pytest plugin reference page to better indicate this is an automated, non-curated listing. ### [`v7.4.1`](https://github.com/pytest-dev/pytest/releases/tag/7.4.1): pytest 7.4.1 (2023-09-02) [Compare Source](pytest-dev/pytest@7.4.0...7.4.1) ## Bug Fixes - [#​10337](pytest-dev/pytest#10337): Fixed bug where fake intermediate modules generated by `--import-mode=importlib` would not include the child modules as attributes of the parent modules. - [#​10702](pytest-dev/pytest#10702): Fixed error assertion handling in `pytest.approx` when `None` is an expected or received value when comparing dictionaries. - [#​10811](pytest-dev/pytest#10811): Fixed issue when using `--import-mode=importlib` together with `--doctest-modules` that caused modules to be imported more than once, causing problems with modules that have import side effects. </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi44LjExIiwidXBkYXRlZEluVmVyIjoiMzYuMTA3LjIiLCJ0YXJnZXRCcmFuY2giOiJtYXN0ZXIifQ==--> Reviewed-on: https://git.apud.pl/jacek/adventofcode/pulls/32 Co-authored-by: Renovate <renovate@apud.pl> Co-committed-by: Renovate <renovate@apud.pl>
In pmxbot/pmxbot@7f189ad, I'm attempting to switch pmxbot off of pkg_resources style namespace packaging to PEP 420 namespace packages. To do so, I've needed to switch to
importlib
for theimport-mode
and re-organize the tests to avoid import errors on the tests.Yet even after working around these issues, the tests are failing when the effect of
core.initialize()
doesn't seem to have had any effect.Investigating deeper, I see that initializer is executed and performs its actions (setting a class variable
pmxbot.logging.Logger.store
), but when that happens, there are two different versions ofpmxbot.logging
present, one insys.modules
and another found intests.unit.test_commands.logging
:I haven't yet made a minimal reproducer, but I wanted to first capture this condition.
The text was updated successfully, but these errors were encountered: