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

Module imported twice under import-mode=importlib #10811

Closed
jaraco opened this issue Mar 12, 2023 · 4 comments · Fixed by #11148
Closed

Module imported twice under import-mode=importlib #10811

jaraco opened this issue Mar 12, 2023 · 4 comments · Fixed by #11148
Labels
type: bug problem that needs to be addressed

Comments

@jaraco
Copy link
Contributor

jaraco commented Mar 12, 2023

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 the import-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 of pmxbot.logging present, one in sys.modules and another found in tests.unit.test_commands.logging:

=========================================================================== test session starts ===========================================================================
platform darwin -- Python 3.11.1, pytest-7.2.0, pluggy-1.0.0
cachedir: .tox/python/.pytest_cache
rootdir: /Users/jaraco/code/pmxbot/pmxbot, configfile: pytest.ini
plugins: black-0.3.12, mypy-0.10.3, jaraco.test-5.3.0, checkdocs-2.9.0, flake8-1.1.1, enabler-2.0.0, jaraco.mongodb-11.2.1, pmxbot-1122.14.3.dev13+g7f189ad
collected 421 items / 180 deselected / 241 selected                                                                                                                       
run-last-failure: rerun previous 240 failures (skipped 14 files)

tests/unit/test_commands.py E
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> traceback >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

cls = <class 'tests.unit.test_commands.TestCommands'>

    @classmethod
    def setup_class(cls):
        path = os.path.dirname(os.path.abspath(__file__))
        configfile = os.path.join(path, 'testconf.yaml')
        config = pmxbot.dictlib.ConfigDict.from_yaml(configfile)
        cls.bot = core.initialize(config)
>       logging.Logger.store.message("logged", "testrunner", "some text")
E       AttributeError: type object 'Logger' has no attribute 'store'

tests/unit/test_commands.py:37: AttributeError
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> entering PDB >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> PDB post_mortem (IO-capturing turned off) >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
> /Users/jaraco/code/pmxbot/pmxbot/tests/unit/test_commands.py(37)setup_class()
-> logging.Logger.store.message("logged", "testrunner", "some text")
(Pdb) logging.Logger
<class 'pmxbot.logging.Logger'>
(Pdb) logging
<module 'pmxbot.logging' from '/Users/jaraco/code/pmxbot/pmxbot/pmxbot/logging.py'>
(Pdb) import sys
(Pdb) sys.modules['pmxbot.logging']
<module 'pmxbot.logging' from '/Users/jaraco/code/pmxbot/pmxbot/pmxbot/logging.py'>
(Pdb) sys.modules['pmxbot.logging'] is logging
False

I haven't yet made a minimal reproducer, but I wanted to first capture this condition.

@jaraco
Copy link
Contributor Author

jaraco commented Jun 25, 2023

In pmxbot/pmxbot@3adc54c, I've managed to pare down the project to a bare minimum reproducer. The issue only happens when import-mode=importlib and doctest-modules and one of the modules imports another module.

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.

@nicoddemus
Copy link
Member

Hey @jaraco, thanks for the reproducer!

I found the problem, will open a PR shortly.

nicoddemus added a commit to nicoddemus/pytest that referenced this issue Jun 29, 2023
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
nicoddemus added a commit to nicoddemus/pytest that referenced this issue Jun 29, 2023
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
@nicoddemus
Copy link
Member

I'm not even aware of a good workaround.

Btw I noticed that if you call pytest tests then it works, even without #11148.

@jaraco
Copy link
Contributor Author

jaraco commented Jun 29, 2023

I'm not even aware of a good workaround.

Btw I noticed that if you call pytest tests then it works, even without #11148.

Yes, but that also effectively disables doctests.

I found the problem, will open a PR shortly.

Neat! I'll test it against the full pmxbot branch that's switching to a native namespace package implementation.

nicoddemus added a commit to nicoddemus/pytest that referenced this issue Jul 1, 2023
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
nicoddemus added a commit to nicoddemus/pytest that referenced this issue Jul 1, 2023
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
nicoddemus added a commit that referenced this issue Jul 1, 2023
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
jsuchenia pushed a commit to jsuchenia/adventofcode that referenced this issue Dec 2, 2023
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

-   [#&#8203;11237](pytest-dev/pytest#11237): Fix doctest collection of `functools.cached_property` objects.

-   [#&#8203;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.

-   [#&#8203;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.

-   [#&#8203;11394](pytest-dev/pytest#11394): Fixed crash when parsing long command line arguments that might be interpreted as files.

### Improved Documentation

-   [#&#8203;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

-   [#&#8203;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.

-   [#&#8203;10702](pytest-dev/pytest#10702): Fixed error assertion handling in `pytest.approx` when `None` is an expected or received value when comparing dictionaries.

-   [#&#8203;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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug problem that needs to be addressed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants