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

Fix error with --import-mode=importlib and modules containing dataclasses or pickle #7870

Merged
merged 11 commits into from
Apr 5, 2021

Conversation

tadeu
Copy link
Member

@tadeu tadeu commented Oct 7, 2020

fixes #7856, fixes #7859

@tadeu tadeu changed the title Fix error with --import-mode=importlib and modules containing dataclasses Fix error with --import-mode=importlib and modules containing dataclasses or pickle Oct 7, 2020
@tadeu tadeu force-pushed the fix-importlib-error branch 2 times, most recently from ac2cc44 to 6dad144 Compare October 7, 2020 11:18
@tadeu tadeu requested a review from nicoddemus October 7, 2020 11:19
@nicoddemus
Copy link
Member

nicoddemus commented 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 importlib should bring to the table: I always figured it was about not changing sys.path and sys.modules, but I might be wrong about the latter.

@nicoddemus
Copy link
Member

(Marking this with the hacktoberfest-accepted label as this is clearly a quality contribution, but I'm afraid it is complex and might still require a bunch of discussions to move forward)

@nicoddemus nicoddemus self-assigned this Dec 9, 2020
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
Copy link
Member

Picked this up again, I've rebased and changed the approach to use a dotted name for modules, anchored to config.rootpath to obtain a unique name.

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

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?

@nicoddemus
Copy link
Member

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 importlib mode, it is a premise that they won't be able to import things from other test modules, so I'm having a hard time coming up with a plausible example where this might be an issue.

@bluetech
Copy link
Member

Got it. If it turns out to be a problem in practice, then worst case we can revert or fix it some other way.

Base automatically changed from master to main March 9, 2021 20:40
@nicoddemus nicoddemus added this to the 7.0 milestone Apr 1, 2021
@nicoddemus
Copy link
Member

nicoddemus commented Apr 2, 2021

Rebased on latest main, needs to be squashed on merge.

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 nicoddemus changed the title Fix error with --import-mode=importlib and modules containing dataclasses or pickle Fix error with --import-mode=importlib and modules containing dataclasses or pickle [needs squash] Apr 2, 2021
@nicoddemus nicoddemus changed the title Fix error with --import-mode=importlib and modules containing dataclasses or pickle [needs squash] [SQUASH] Fix error with --import-mode=importlib and modules containing dataclasses or pickle Apr 2, 2021
@nicoddemus nicoddemus changed the title [SQUASH] Fix error with --import-mode=importlib and modules containing dataclasses or pickle Fix error with --import-mode=importlib and modules containing dataclasses or pickle Apr 5, 2021
@nicoddemus nicoddemus merged commit b706a2c into pytest-dev:main Apr 5, 2021
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
@The-Compiler The-Compiler modified the milestones: 8.0, 7.0 Jun 29, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pytest importlib doesn't work with pickle pytest with importlib & dataclass & __future__ annotations crash
6 participants