-
-
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
[BLOCKED] --import-mode=importlib: use sys.path relative path if present (wait for #7870) #7936
Closed
+67
−5
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment above here, explaining with this code block is needed? IIUC, what it does is "If the path is importable as a name from the search path, then import it as such.". But the comment should say why it's done.
Also, AFAIK (not 100% sure) the previous importlib code doesn't insert the imported module into
sys.modules
, butimportlib.import_module
does. I think the idea was to not insert it, as mentioned in the--import-mode=importlib
changelog entry:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing, very happy to add a comment. I'll push it as a separate commit.
This is true. I noticed that #7870 was actually proposing to change that though so that the imported module is added to
sys.modules
(to fix the issues noted there).An interesting thing to note about this approach is that the entry that ends up in
sys.modules
will be the same as ifimport foo.bar.test_baz
was executed - e.g.sys.modules["foo.bar.test_baz"]
.With the approach in #7870 the concern is that test modules which have conflicting names will again have issues, as described in the changelog entry you quote. (The code in #7870 would add just
test_baz
tosys.modules
, for example.)This approach would add the fully qualified name
foo.bar.test_baz
tosys.modules
, which may be interesting as an alternative approach to the fix in #7870.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation.
So the approach does sounds appealing, but it does bring us again to have a dependence on
sys.path
and how it is configured (e.g. if it contains the test directory, at what depth, etc.). Basically it becomes pretty similar to--import-mode=append
, no? Just without keeping thesys.path
dirty.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's a very fair assessment of this approach.
Given that it's perhaps markedly different from the
importlib
mode, perhaps it makes more sense to make this a new mode, maybe calledpath
?That way it'll have reliable behaviour - if the test file is not underneath a path entry, pytest will not be able to import it. So users who want to test using this mode will have to add the test files to their path (usually by
setup.py develop
).The advantages of it are primarily for test files inside packages, so I don't think that's an unreasonable requirement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @davidhewitt for the patch! This is complicated topic so we are very grateful for the attempt.
This is an interesting approach, but doesn't handle one of the main uses of
--importmode=importlib
, which is using it to collect test files which are not insys.path
at all. Consider this common layout:In this case
src
is added tosys.path
due topython setup.py develop
(or adding to$PYTHONPATH
manually), however we don't have any parent oftests/io/test_measurement.py
andtests/model/test_measurement.py
insys.path
, which wouldn't trigger your patch and would still cause the issues withpickle
/dataclasses
mentioned in the original issue.Your patch would work fine though when the tests are siblings of an entry in
sys.path
, which is common for "in-source" test layouts:But again this is not one of the use cases that
importlib
mode attempts to solve.That's sensible, but I'm not sure it is the best approach. Supposing we can fix #7870, what would be the advantage of
path
overimportlib
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As well as #7870 I have a separate issue with
importlib
mode, which is that relative imports no longer work inimportlib
mode. This patch originated as a attempt to figure out how to get__package__
correctly set as suggested at #7245 (comment).I'm happy to close this PR if it doesn't look like the right solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidhewitt again thanks for the patch!
Let's keep this open until we get #7870 working (even if only as a reference): we can then see if it also solves the relative import problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Note that the test case I've written here is particularly awkward but is amazingly also a real-world use case I'm suffering with which motivated me to write the PR.
I'm attempting a relative import from a test file that is inside a pep 420 namespace package, where the first non-implicit package is called
csv
, and so collides with the builtin package of the same name.Interestingly the
append
mode also cannot handle this case correctly, so I was wondering if this patch could also be relevant for theappend
mode.