Fix error with --import-mode=importlib and modules containing dataclasses or pickle #7870
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tadeu
force-pushed
the
fix-importlib-error
branch
from
abcc1cf
to
c08f4d1
Compare
tadeu
changed the title
--import-mode=importlib and modules containing dataclasses--import-mode=importlib and modules containing dataclasses or pickle
tadeu
force-pushed
the
fix-importlib-error
branch
2 times, most recently
from
ac2cc44
to
6dad144
Compare
nicoddemus
reviewed
Oct 7, 2020
|
Thanks a lot @tadeu! I'm not 100% sure the fix is correct (or even if it is fixable), but let see what the other maintainers think. I might have the wrong idea about what benefits |
|
(Marking this with the |
Closed
5 tasks
facebook-github-bot
pushed a commit
to facebookresearch/beanmachine
that referenced
this pull request
Dec 11, 2020
Summary: Pull Request resolved: #477 As titled. Currently pytest's `import-mode=importlib` option does not add the test file to `sys.module`, which can break some of the use cases. While [the community has came up with a fix](pytest-dev/pytest#7870), it's not yet clear when the particular PR will be merged and added to the latest pytest. So in the meanwhile, we could download and use the patched version by directly from the PR so we are not blocked. Reviewed By: ericlippert Differential Revision: D25480894 fbshipit-source-id: 5eddce784a247f68ebcaa5ae85065220f65d847a
nicoddemus
force-pushed
the
fix-importlib-error
branch
from
6dad144
to
ebc2622
Compare
|
Picked this up again, I've rebased and changed the approach to use a dotted name for modules, anchored to |
bluetech
reviewed
Dec 19, 2020
There was a problem hiding this comment.
I haven't reviewed in depth but LGTM.
One thing I wonder is, can the empty modules added by insert_missing_modules cause any trouble? If a module name already exists, it doesn't override it, which is good, but I wonder about the opposite case, when insert_modules inserts an empty module, and after that a real module with that name wants to be imported. Is this case possible? And if so, what will happen?
Hmm good points, I think it will blow up, as someone might be expecting the real module (to say, call a function from it) and they will get a fake module instead. However I think this is highly unlikely, because if one is using |
|
Got it. If it turns out to be a problem in practice, then worst case we can revert or fix it some other way. |
bluetech
approved these changes
Dec 20, 2020
nicoddemus
force-pushed
the
fix-importlib-error
branch
from
a1b3c0c
to
91661a0
Compare
|
Rebased on latest Sorry for dropping the ball on this one, this totally got out of my radar. I will merge it tomorrow unless someone wants more time to give it another pass, in which case just let me know here. |
nicoddemus
changed the title
--import-mode=importlib and modules containing dataclasses or pickle--import-mode=importlib and modules containing dataclasses or pickle [needs squash]
nicoddemus
changed the title
--import-mode=importlib and modules containing dataclasses or pickle [needs squash]--import-mode=importlib and modules containing dataclasses or pickle
nicoddemus
changed the title
--import-mode=importlib and modules containing dataclasses or pickle
supercerealized
added a commit
to supercerealized/jobcoin-mixer
that referenced
this pull request
Apr 18, 2021
…rt mode going forward isn't certain so avoiding relative imports with pytest seems more extensible pytest docs on this:https://docs.pytest.org/en/stable/goodpractices.html#tests-outside-application-code. Pytest dev branch discussion around this: pytest-dev/pytest#7245 AND pytest-dev/pytest#7870
saimn
added a commit
to saimn/pytest-doctestplus
that referenced
this pull request
Jun 13, 2021
nicoddemus
added a commit
to nicoddemus/pytest
that referenced
this pull request
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 pull request
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
added a commit
to nicoddemus/pytest
that referenced
this pull request
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 pull request
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 pull request
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #7856, fixes #7859