Add an option to disable sys.path patching. #5235
Conversation
Pull Request Test Coverage Report for Build 1409090769
💛 - Coveralls |
Pierre-Sassoulas
added
Enhancement ✨
cdce8p
removed
the
Needs review 🔍
|
Testing wise I tested this on my team's codebase that had import namespace package error and it fixed issue for us. Documentation, @Pierre-Sassoulas where would you recommend me updating the documentation? The faq the run page with other global options or somewhere else? If I do the faq I could add a question under troubleshooting for namespace packages. |
|
In the faq is a really good idea :) Also if you have ideas about how to test that automatically with a minimum namespace package so we don't regress later on that would be great, but I'm sure this is quite complicated to do. |
|
I wasn't sure if I could write a unit test, but I think I got a reasonable one to work using functional test. It does rely on pylint being installed when running test for disable path patching one to work or running from the repository root directory. So locally testing it with an editable pylint install it worked. I'm unsure if CI installs pylint before testing or not. If that doesn't work on CI then I think CI will need a tweak. I'll add documentation tomorrow. I think best test though for this would be part of pylint primer and include some repository that uses namespace packages and require disable-path-patching to work correctly. Ideal repository is one with multiple python packages (monorepo) in it with at least one namespace package that overlaps in name with other package in repo if you ignore namespace part. |
|
I'm on mobile, so I can't link the issue but one of the last things we need to do for the 2.12 milestone is (convenientely) adding primer tests. Please feel free to suggest repositories that would be great to add on the relevant issue (you can click the milestone from here). |
|
The issue regarding the primer tests Pierre mentioned |
|
Test failure is interesting. The two functional tests I wrote pass individually, but fail when run together. Awkwardly once an import happens it impacts global state (sys.modules) and influences next test import resolution. Messy aspect is this can negatively impact an entirely unrelated test. My test that relied on import failure for namespace_package since it used checkers broke generated_members test because it also imported checkers. After a couple tries I got it working with all tests and left a long comment in _fake.py about it. |
| @@ -0,0 +1,7 @@ | |||
| """This file is intended to only be used for testing namespace package importing. | |||
There was a problem hiding this comment.
I don't think we should add file without purpose in pylint itself. pylint is not a namespace package so maybe we can create a fixture or it's not even required to have one ? Could we use a namespace that does not exists ? Or is this supposed to be in tests/functional/n/namespace_package/pylint/testutils/ ?
There was a problem hiding this comment.
I thought testutils was for files used in tests/? Are you fine with test file importing another test file? I don't think that's currently done by any of the tests and a future version of pytest will not allow that by default either.
https://docs.pytest.org/en/6.2.x/pythonpath.html#import-modes
For this reason this doesn’t require test module names to be unique at all, but also makes test modules non-importable by each other
So moving it to tests/functional/n/namespace_package will force you to disable an upcoming pytest change.
There's a second reason why it's better to avoid having the file in tests/. sys path disable mode has a requirement either files you import are installed or you run it from the right directory. tests folder is not included in pytest install. As a side effect if I move the file to tests this new test will only work as expected if run from the repository root directory. If a user tries to run pytest from a different directory for this test it won't work.
Because of those 2 reasons having the _fake file with tests/ will impose restrictions on pytest usage and I'd prefer to avoid. I can move it there, but the tests will become sensitive to where you run them and unsure that's a good trade off.
There was a problem hiding this comment.
Other option is drop this test entirely and make sys path logic is tested in future primer test.
The main issue for making this not directory sensitive is I need module I'm importing to be installed package in repo. The only choice for that is under pylint/ somewhere. I can make a folder _internal/ and actual location doesn't matter as long as it's in pylint.
There was a problem hiding this comment.
Sorry I forgot to reply. We have some test with import in test_func.py which is the legacy functional test file. Maybe you could take add a test to it exceptionally ? I'm guessing we'll have to think about a system for those kind of tesst soon if pytest is going to make this impossible. But it will always be possible to call pylint on a directory, as an integration tests, right ? Sorry if this is not in this PR scope. I'm hesitant to drop the test as long as #5173 is not merged. Do you have a list of stable project that we could use to test this ?
There was a problem hiding this comment.
Would putting it in tests/regrtest_data work? This is where I have been putting all data for tests that require additional files.
We should probably update the name of that directory though...
There was a problem hiding this comment.
No it would have same issue as anywhere outside pylint. The root issue is this line, https://github.com/PyCQA/pylint/blob/main/setup.cfg#L57
When pylint is installed as a package, it installs pylint directory. It does not install tests directory. Namespace package solution here works under one of 2 conditions, the imported modules are installed or you need to use right directory to run them from. The second condition means that for development tests will break depending on cd and will complicate test scripts. The first condition means I can't test this well if pylint separates test folder from the install.
Some codebases have a test structure of keeping tests in same folder as src and tests are included with a package install. https://docs.pytest.org/en/6.2.x/goodpractices.html#tests-as-part-of-application-code pytest discusses different test folder structures here. The structure of mixing tests with application code would work. It'd need a pr to refactor the test structure. You could in theory mix both test structures but it'd be fairly confusing if some tests lived in pylint/ and other tests lived in tests/. Having every user be forced to install all tests as part of pip install is a downside to this option.
For stable project to test with I'm unsure of repository that uses implicit namespace packages and would fail without this. Most repositories don't use namespace packages. For the ones that do I expect if they use pylint they have workarounds. My internal repo that needs this uses a workaround of never calling pylint on the whole repo and also has multiple setup.py in it which primer would struggle with. The primer I think is designed for repositories with single setup.py/cfg. Monorepo that has multiple separate packages (so separate setup.cfg/py) installed to same namespace I don't think mypy primer supports and that's situation most likely to need this.
I can make a toy repository for this that primer would work with.
There was a problem hiding this comment.
Would this work for primer ? https://github.com/madhavhugar/example-python-namespace-package
There was a problem hiding this comment.
No. Reading the project,
When using multiple namespace packages within the same repository:
Because mynamespace doesn’t contain an init.py, setuptools.find_packages() won’t find the sub-package. You must use setuptools.find_namespace_packages() instead or explicitly list all packages in your setup.py. For example:
is true in readme, but the repository does not follow that itself.
https://github.com/madhavhugar/example-python-namespace-package/blob/master/setup.py#L11
I don't see any namespace module in that repository. Instead it looks like a repository that has 3 packages all in same repository and controlled by same setup.py, but no namespace that shares them.
I can make a simple example for the primer tomorrow.
There was a problem hiding this comment.
Thank you much appreciated !
|
@Pierre-Sassoulas Thoughts on what I should do with the test? Main options I see are drop the test or keep a file somewhere in pylint/. I don't see a good way to test importing works with disable path on and having the files all in tests/ directory. |
Sorry for the delay. Imo we can drop the tests if we introduce a namespace repository with #5173 |
|
@hmc-cs-mdrissi did you have the time to find an example of a namespace package ? We're trying to release 2.12 this week, and there's only this issue and the primer tests left :) |
DanielNoord
added
the
Waiting on author
Looks good 👍 (We just need to check that it was failing previously) @hmc-cs-mdrissi do you still want to work on this or should we take over ? I think this MR is very valuable and need to go in 2.13 :) |
|
@hmc-cs-mdrissi Sorry for pinging you again. As @Pierre-Sassoulas said previously this is likely a much appreciated function for many users. Do you think you have the time to write the small package for the |
Pierre-Sassoulas
removed
the
Waiting on author
update changelog + contributors Address comment to add default for option Add unit test for namespace package sys patching fix unit test to avoid global side effects from imports Update faq to explain namespace package issue with imports
Pierre-Sassoulas
force-pushed
the
md/add_option_disable_sys_path_patching
branch
from
7454a02 to
52738b6
Compare
|
Confirmed to fix namespace issue in #2648 (comment) |
Pierre-Sassoulas
added
the
Needs backport
|
@Pierre-Sassoulas I think I have found a solution for namespaces without this option. Using some of the import logic we have already made at Let's wait (again, I know...) with merging this... |
|
Closing in favor of #6405 |
|
Thank you for working on this @hmc-cs-mdrissi, even if we did not merge the change in the end it would have been a great contribution and made us rethink the way we handle namespace in pylint. |
Pierre-Sassoulas
removed
the
Needs backport
Type of Changes
Description
Adds an option for disabling sys.path patching. This option is default to False and should have no impact on existing users.
This is helpful for implicit namespace packages where the current patching logic is incorrect and making patch logic correct in general case is difficult. Disabling sys.path patching means that pylint will work if running python on same files will work. Practically this means any files users want to check should either be checked from the package root directory or be installed first.
Closes #5226