-
-
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
FIX key formating divergence when inspecting plugin dictionary. #11708
FIX key formating divergence when inspecting plugin dictionary. #11708
Conversation
Also fixes scikit-learn/scikit-learn#27806 |
1ff7cda
to
44c0319
Compare
If adopted it should be backported to latest supported versions, since this bug has been reported in |
Thanks for tracking down this sneaky problem @fcharras. My main initial request is: can you add a new, standalone test which reproduces the issue on Windows (i.e. fails before the fix, and passes after)? And what happens if you run pytest like |
Would it be enough to do a simple unit test, i.e. something like registering a plugin with upper-case, querying the plugin dict with lower-case and making sure that it is found? Would you rather have something closer to an integration test that looks like the original bug #9765. The latter is probably significantly harder to put together.
The issue is still there with PYTHONCASEOK=1. |
src/_pytest/config/__init__.py
Outdated
@@ -744,9 +746,9 @@ def consider_pluginarg(self, arg: str) -> None: | |||
del self._name2plugin["pytest_" + name] | |||
self.import_plugin(arg, consider_entry_points=True) | |||
|
|||
def consider_conftest(self, conftestmodule: types.ModuleType) -> None: | |||
def consider_conftest(self, conftestmodule: types.ModuleType, name: str) -> None: |
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.
strictly speaking this one is a "breaking api change" do we have any idea on the impact - i suspect its zero, but i'm not certain
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.
Would it fix to have name
default to None
and use conftestmodule.__file__
(maybe normalized ) if so ?
Or, another way I considered would be to run mod = import_path(conftestpath, mode=importmode, root=rootpath)
first, then using mod.__file__
to pass the existing
check rather than str(conftestpath)
: that works too, with the advantage of not changing the api, and not altering the keys in name2plugins
compared to current. The downside is that import_path
is now also called for plugins that are already registered (which might not be important since the module should be already imported so the import_module
will do nothing ? but I wasn't sure of that).
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.
The first way sounds practical,
I'd recommend normalizing
I'd defer implementation, in Case there's next to no impact it should go into 8.x as is and the backport would trigger a deprecation warning plus use none
Adressed comment + best-effort attempt at non-regression tests. (see #11725 ) |
I left a comment on the issue: #9765 (comment)
Basically yes -- doesn't need to be an integration test in pytest test suite, but just an end-to-end reproduction. The unit tests in this PR are useful to prevent regressions, but not for understanding the issue because they already pass the two differing paths as inputs.
Thanks for testing. This is on Windows right? So it may indicate that the issue also in your case is not merely with case but also normalization. @fcharras It will be helpful if you could split the change to two commits: one which does the |
src/_pytest/config/__init__.py
Outdated
@@ -639,7 +639,11 @@ def _rget_with_confmod( | |||
def _importconftest( | |||
self, conftestpath: Path, importmode: Union[str, ImportMode], rootpath: Path | |||
) -> types.ModuleType: | |||
existing = self.get_plugin(str(conftestpath)) | |||
conftest_registration_name = str( |
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.
The str
here is not necessary I believe.
I put together a reproducer repo that reproduces (Windows-only) https://github.com/lesteve/pytest-issue-9765-reproducer. For some reason you need the combination of the three following things in our case for the issue to appear:
Not sure how easy this is to turn the repo into a pytest test.
Yes
In the case we looked at together with @fcharras (Windows-only), I think we only need |
It seems like a user is experiencing the issue on Linux with normalization, see #9765 (comment) , but we don't know how to reproduce. It's likely that I don't know the pytest code base enough in depth in order to propose an accurate fix. If this PR did target the root of the issue, that would be nice, but I don't have a big picture. What I understand of the issue is that there is a dict ( However, I discovered that it is indeed possible to import a same module several times by registering it with different paths, it sounds crazy but maybe there are valid usecases with that that you want to support ? It's beyond what I am able to do as far as crafting reproducers go or thinking about exotic use-cases. But since all tests pass on this branch already and that it seems to cover a private area of the codebase ( |
I have added a PR on @fcharras fork to add a test based on my reproducer repo in fcharras#1 (a PR on top of this PR). For some reason I don't understand the error when running this additional test is not exactly the same (
My personal take on this PR:
|
testing/acceptance_test.py
Outdated
) | ||
) | ||
|
||
pytester.run(sys.executable, "setup.py", "develop") |
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.
setup.py develop has been deprecated due to numerous issues with it
does the issue only happen with legacy editables, or does it also happen with modern editables?
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.
I know but this is the we originally had the issue in scikit-learn, and we manage to put together a reproducer. This only happens with legacy editable install python setup.py develop
or its pip equivalent pip install --no-use-pep517 -e .
Note that other projects (scipy see #9765 (comment) and jdaviz #9765 (comment) for now but I could magine others) seem to have a very closely related problems in pytest 8 release candidate, due to normcase path difference on Windows. This does not seem to involve legacy editable installs, so this is a more general issue than this.
Note although this is somewhat deprecated, we still recommend to use pip install --no-use-pep517
for scikit-learn contributors. The incremental builds do not seem to work with the non-legacy installs so you rebuild everything from scratch. If you really want to know more, look at for example scikit-learn/scikit-learn#27960
testing/acceptance_test.py
Outdated
""" | ||
) | ||
) | ||
Path("pytest.ini").write_text( |
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.
those paths are relative to cwd
if there is a need to create a python package and doing a editable install of it,
i'm under the impression we need to rework how things are called a little
ba9f654
to
31a4291
Compare
OK in 31a42915bbcf6d59a1236624549f8bdd2723e95aI managed to reproduce the In my last commit, everything should be green, except coverage maybe (this is because the I tried to simplify the code diff trying to follow "solution 1." in #9765 (comment). About the added tests:
|
Thanks for the update @lesteve! This solution has the problem described under "Solution 2":
Further, I think we should completely avoid dependence on Regarding the regression test which does
Unless we can make the test independent of setuptools stuff, we can go without a regression test for now. |
ff303fc
to
b940999
Compare
Rereading your summary post, I think indeed you are right in the sense that I kept using In my last commit I tried to do something closer to what you had in mind in solution 1. The questions I have:
I kind of think the registration key should be normalized, but that means you need to normalize it too when querying the dict. If I understand you correctly you think it is fine to not normalize it and use I get the worry about using deprecated functionality, but my feeling is the following: this is used for a good reason (see #11708 (comment)) by a well-managed project like scikit-learn, so there are chances that it is used in the wild by a number of other projects. I would also expect the setuptools deprecation to take a while (years?) before the warning is turned into an error, but I did not find anything precise about this. Also this is the only realistic test we have so far, personally I would lean towards including it, rather than flying blind. This would be completely fine to remove it when it becomes too much of a maintenance burden e.g. I agree about test isolation but I suppose this could be worked around by removing |
126b151
to
6c3cac3
Compare
6c3cac3
to
b816b3a
Compare
@jgb by the way if you want take the time to put together a reproducer for your issue on Linux and even better a pytest test (but that's not the hardest part), that would be super useful 🙏. As you might have seen from the discussion, the only reproducer is on Windows and it is under discussion whether it is going to be included in this PR or not because it is using deprecated |
src/_pytest/config/__init__.py
Outdated
if registration_name is None: | ||
warnings.warn( | ||
"'consider_conftest' was called with registration_name=None, this will become an error in the future'", | ||
PytestDeprecationWarning, | ||
) | ||
# Keep Pytest < 8 previous behaviour | ||
registration_name = conftestmodule.__file__ |
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.
I read the discussion, but to be honest I don't think we need to deprecate receiving None here.
@RonnyPfannschmidt what issue do you see that could be a problem accepting None
as registration_name
, and fallback to the previous behavior, which is what has been used for ages now?
If we feel strongly that the new parameter is needed, giving that it seems like it would break very little plugins that we know of and the "fix" is trivial, I would rather we just break the API then, but I'm not convinced that we need to, accepting None
and fallback to the previous behavior seems acceptable to me.
So my preference would be (in order):
- Use
None
as default forregistration_name
and fallback to the previous behavior. - Break the API and make
registration_name
mandatory.
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.
I think we should go for 2, that is break without deprecation:
- The function is not documented in the API Reference and is marked
:private:
- Searching my corpus of pytest plugins finds no matches
- Searching github finds no matches (other than pytest itself and copies of it)
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.
Sounds good to me. 👍
src/_pytest/config/__init__.py
Outdated
@@ -634,7 +635,8 @@ def _rget_with_confmod( | |||
def _importconftest( | |||
self, conftestpath: Path, importmode: Union[str, ImportMode], rootpath: Path | |||
) -> types.ModuleType: | |||
existing = self.get_plugin(str(conftestpath)) | |||
conftestpath_str = str(conftestpath) |
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.
I think we should create a function:
def get_conftest_plugin_name(p: Path) -> str:
return str(p)
and reuse that in both places here. This makes it standout more, plus we can document the issue (and reference the issue number) in the get_conftest_plugin_name
docstring.
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.
Sorry, I am not sure I understood what you meant by "reuse that in both places here" since so for now I only renamed the variable to be more explicit confestpath_str
-> conftest_plugin_name
.
Maybe you were suggesting something like this?
existing = self.get_plugin(get_conftest_plugin_name(conftestpath)
...
self.consider_conftest(mod, registration_name=get_conftest_plugin_name(conftestpath))
Both places are in the same function, so it feels slightly weird to call the helper function twice but maybe that is indeed what you wanted, anticipating further use of the get_conftest_plugin_name
in other places of the code in the future?
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.
My intention is that having a function we make this issue more explicit, is easy to miss the significance of that tiny str
call and why it is so important in the first place. Also the function lets us add more documentation/context.
But nevermind, given this is only local to the function I think the variable name is enough to convey the importance of that str
call and it being reused in the self.consider_conftest
.
src/_pytest/config/__init__.py
Outdated
if registration_name is None: | ||
warnings.warn( | ||
"'consider_conftest' was called with registration_name=None, this will become an error in the future'", | ||
PytestDeprecationWarning, | ||
) | ||
# Keep Pytest < 8 previous behaviour | ||
registration_name = conftestmodule.__file__ |
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.
I think we should go for 2, that is break without deprecation:
- The function is not documented in the API Reference and is marked
:private:
- Searching my corpus of pytest plugins finds no matches
- Searching github finds no matches (other than pytest itself and copies of it)
config.pluginmanager._name2plugin[str(conftest_upper_case)] is mod_uppercase | ||
) | ||
|
||
# No str(conftestpath) normalization so conftest should be imported |
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.
I'll note that this is not the behavior we want -- we want them to be the same. But as we agreed I will add the normalization in a separate PR.
@@ -1390,3 +1392,63 @@ def test_boo(self): | |||
) | |||
result = pytester.runpytest_subprocess() | |||
result.stdout.fnmatch_lines("*1 passed*") | |||
|
|||
|
|||
def test_issue_9765(pytester: Pytester) -> None: |
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.
def test_issue_9765(pytester: Pytester) -> None: | |
@pytest.mark.skip(reason="Test is not isolated") | |
def test_issue_9765(pytester: Pytester) -> None: |
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.
I skipped the test for now, but I may consider doing a follow-up PR to add venv creation and activation in this test, via python -m venv
+ activate
, would you be OK with this?
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.
venv creation is quite heavy and slow, and the setuptools thing still remains, so I'd still prefer it skipped
I went for the API break in my last commit. As I mentioned above in #11708 (comment) I found at least one project (CatBoost) that was using I haven't added a changelog, but feel free to push into this branch as you see fit. I am pretty sure @fcharras would not mind either 😉. |
Thanks!
Done. 👍 |
I think this warrants a backport to |
Fix #27806. Co-authored-by: Loïc Estève <[email protected]> Co-authored-by: Ran Benita <[email protected]> Co-authored-by: Bruno Oliveira <[email protected]>
facd721
to
203889b
Compare
I've squashed the commits to one with some minor cleanups to the tests, and added a commit with the "solution 3" normalization (under my own name so as not to implicate @fcharras and @lesteve in it 😄 ). If all of the tests pass, I will merge & backport it. (Later) the tests fail so I'll do it in a separate PR anyway... |
203889b
to
a7c2549
Compare
Thanks everyone for the hard work here! |
Great to see this one merged 🎉, thanks for steering this PR to completion! |
Should we backport this to 8.x? |
I am planning to do 8.0.0rc2 sometime this week. |
Sounds great, I don't think this is in any way urgent (e.g. today vs Friday does not make a huge difference), but it would be certainly be very useful to double-check that it fixes the issue for the affected projects. |
Oops missed that |
Thank you for contributors and maintainer that took the PR much farther than I initiated and making the merge happen ❤️ @jgb from our understanding now it's likely that despite reverting part of the initial pr (the "normpath" call) the PR fixed the issue on linux too. |
Fixes #9765
What happens is that
str(conftestpath)
is not necessarily equal tomod.__file__
, which is the key that is really used inself._name2plugin
. For instance I've noticed that on windows, themodule.__file__
string for some modules is sometimes small cased (c:\\users...
), whilestr(conftestpath)
is not (C:\\Users...
). For my particular reproducer of #9765, it causes some plugins loaded from conftest to be double loaded, since the preliminary lookup fails because the key is wrong. And double-loading leads to a crash.This might not be a good fix if there are other direct calls to
self.register
in other areas of the code base that would risk to format the keys differently when it's crafted from paths to conftest files, but I've not found any evidences of that.