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

Pylint takes the isort configuration into account only if it's directly available in its launch directory #4437

Open
fphammerle opened this issue May 3, 2021 · 21 comments
Labels
Enhancement ✨ Improvement to a component Needs PR This issue is accepted, sufficiently specified and now needs an implementation

Comments

@fphammerle
Copy link

fphammerle commented May 3, 2021

Steps to reproduce

  1. Create two python packages apple and banana that share a common namespace fruit. banana is a dependency of apple.

  2. Create a module in apple with the following code:

# apple-package-root/fruit/apple/statistics.py

import math # std lib

import fruit.banana.data # third party
import numpy # third party

import fruit.apple.data # first party
  1. Sort the mentioned module with isort (no change)

  2. Run pylint

Current behavior

fruit/apple/statistics.py:7:0: C0412: Imports from package fruit are not grouped (ungrouped-imports)

Expected behavior

No warning / error

Explanation

In contrast to isort (the command-line tool), pylint incorrectly classifies import fruit.banana.data as a first party import.

This bug results from pylint calling isort_driver.place_module("fruit") (returning "first party") instead of isort_driver.place_module("fruit.banana.data") (would return "third party"):
https://github.com/PyCQA/pylint/blob/v2.8.2/pylint/checkers/imports.py#L723

@Pierre-Sassoulas
Copy link
Member

Thank you for analyzing this problem !

@Pierre-Sassoulas Pierre-Sassoulas added Good first issue Friendly and approachable by new contributors Help wanted 🙏 Outside help would be appreciated, good for new contributors labels May 3, 2021
@yushao2
Copy link
Contributor

yushao2 commented May 9, 2021

take

@yushao2
Copy link
Contributor

yushao2 commented May 9, 2021

Steps to reproduce

  1. Create two python packages apple and banana that share a common namespace fruit. banana is a dependency of apple.
  2. Create a module in apple with the following code:
# apple-package-root/fruit/apple/statistics.py

import math # std lib

import fruit.banana.data # third party
import numpy # third party

import fruit.apple.data # first party
  1. Sort the mentioned module with isort (no change)
  2. Run pylint

Current behavior

fruit/apple/statistics.py:7:0: C0412: Imports from package fruit are not grouped (ungrouped-imports)

Expected behavior

No warning / error

Explanation

In contrast to isort (the command-line tool), pylint incorrectly classifies import fruit.banana.data as a first party import.

This bug results from pylint calling isort_driver.place_module("fruit") (returning "first party") instead of isort_driver.place_module("fruit.banana.data") (would return "third party"):
https://github.com/PyCQA/pylint/blob/v2.8.2/pylint/checkers/imports.py#L723

After some investigation it seems like they are all returning third party? could you share file contents of fruit.apple.data / fruit.banana.data?

@yushao2
Copy link
Contributor

yushao2 commented May 9, 2021

I tried my hands at creating a test case to recreate the issue,

image

but it seems that functional is being recognised as third-party here.

@lukasbindreiter
Copy link

I'm having the same issue as @fphammerle and am also able to reproduce it using your example above.

Given the following ungrouped_imports_namespace.py file (which was sorted using isort):

# pylint: disable=missing-docstring,unused-import

import functional.w.wrong_import_order  # third party
import numpy  # third party

import functional.u.unbalanced_tuple_unpacking  # first party

I'm getting the following pylint output:

************* Module functional.u.ungrouped_imports_namespace
functional/u/ungrouped_imports_namespace.py:4:0: C0411: third party import "import numpy" should be placed before "import functional.w.wrong_import_order" (wrong-import-order)
functional/u/ungrouped_imports_namespace.py:6:0: C0412: Imports from package functional are not grouped (ungrouped-imports)

------------------------------------------------------------------
Your code has been rated at 3.33/10 (previous run: 3.33/10, +0.00)

The directory structure for this example looks like this:

.
├── Pipfile
├── Pipfile.lock
├── README.md
├── functional-u
│   ├── functional
│   │   └── u
│   │       ├── __init__.py
│   │       ├── unbalanced_tuple_unpacking.py
│   │       └── ungrouped_imports_namespace.py
│   └── setup.py
└── functional-w
    ├── functional
    │   └── w
    │       ├── __init__.py
    │       └── wrong_import_order.py
    └── setup.py

6 directories, 10 files

@yushao2 I uploaded the above example to this repository, can you try if you can reproduce the issue there?

@yushao2
Copy link
Contributor

yushao2 commented May 12, 2021

I'm having the same issue as @fphammerle and am also able to reproduce it using your example above.

Given the following ungrouped_imports_namespace.py file (which was sorted using isort):

# pylint: disable=missing-docstring,unused-import

import functional.w.wrong_import_order  # third party
import numpy  # third party

import functional.u.unbalanced_tuple_unpacking  # first party

I'm getting the following pylint output:

************* Module functional.u.ungrouped_imports_namespace
functional/u/ungrouped_imports_namespace.py:4:0: C0411: third party import "import numpy" should be placed before "import functional.w.wrong_import_order" (wrong-import-order)
functional/u/ungrouped_imports_namespace.py:6:0: C0412: Imports from package functional are not grouped (ungrouped-imports)

------------------------------------------------------------------
Your code has been rated at 3.33/10 (previous run: 3.33/10, +0.00)

The directory structure for this example looks like this:

.
├── Pipfile
├── Pipfile.lock
├── README.md
├── functional-u
│   ├── functional
│   │   └── u
│   │       ├── __init__.py
│   │       ├── unbalanced_tuple_unpacking.py
│   │       └── ungrouped_imports_namespace.py
│   └── setup.py
└── functional-w
    ├── functional
    │   └── w
    │       ├── __init__.py
    │       └── wrong_import_order.py
    └── setup.py

6 directories, 10 files

@yushao2 I uploaded the above example to this repository, can you try if you can reproduce the issue there?

Thank you so much for the example! I'm just struggling to figure out how to make functional test cases to reproduce this issue on the repo at the moment :/

If there are any one else interested to work on resolving this bug please go ahead with it!

@doublethefish
Copy link
Contributor

doublethefish commented Nov 5, 2021

I assume that I have missed something as I cannot repro the OP? For me, isort does make modifications and does match pylint's behaviour.

If I read PEP-402 (Simplified Package Layout and Partitioning), PEP-420 (Implicit Namespace Packages) and PEP-561 (Distributing and Packaging Type Information) this is the behaviour we would expect. If we treat a namespace as a collection of modules e.g. fruit in this case, then when importing from the group the intuitive thing is for the imports from that namespace to be grouped. Otherwise, why have the namespace at all? Additionally, modules inside that namespace, that are importing using that namespace, should consider those imports to be sibling imports i.e. First Party - again that feels intuitive to me.

The OP report said that isort and pylint behave differently; the following should show that they don't, but I may have missed something. Note that I've tried to be much more complete in the repro steps than OP, so if I have missed something it is because the OP report wasn't complete or detailed enough.

Setup

If I have a directory structure as follows:

.
├── fruit1  # namespace module _without_ top-level __init__
│   ├── apple
│   │   ├── __init__.py
│   │   └── data.py
│   └── banana
│       ├── __init__.py
│       └── data.py
├── fruit2  # NOTE: no __init__ file anywhere in tree
│   ├── apple
│   │   ├── apple.py
│   │   └── data.py
│   └── banana
│       ├── banana.py
│       └── data.py
└── fruit3  # NOTE: top-level __init__ file only
    ├── __init__.py
    ├── apple
    │   ├── apple.py
    │   └── data.py
    └── banana
        ├── banana.py
        └── data.py

And file contents as follows:

# fruit[1, 2 or 3]/apple's __init__.py or apple.py: 
import math  # std lib

import fruit[1, 2 or 3].banana.data  # update the fruit number for each test candidate
import numpy

import fruit[1, 2 or 3].apple.data
# fruit[1, 2 or 3]/apple/data.py
data = "some apple data"
# fruit[1, 2 or 3]/banana's __init__.py or banana.py: 
# empty file
# fruit[1, 2 or 3]/banana/data.py
data = "some banana data"

Results (isort)

When I run isort "." from "." (and from each of the subdirectories) all the imports in the apple files (fruit1/apple/__init__.py, fruit2/apple/apple.py and fruit3/apple/apple.py) get reordered, contrary to the OP. What did I miss?

@@ -2,7 +2,7 @@

 import math

-import fruit3.banana.data
 import numpy

 import fruit3.apple.data
+import fruit3.banana.data

Results (pylint)

Running pylint --rcfile=.pylint.rc $(fd -e py | ag fruit[1,2 or 3]), from "." or from "fruit[1, 2 or 3]", on an unsorted version of the code (i.e. before running isort) we get:

# fruit1
************* Module apple
fruitX/apple/__init__.py:5:0: E0401: Unable to import 'fruitX.banana.data' (import-error)
fruitX/apple/__init__.py:8:0: E0401: Unable to import 'fruitX.apple.data' (import-error)
fruitX/apple/__init__.py:6:0: C0411: third party import "import numpy" should be placed before "import fruitX.banana.data" (wrong-import-order)
fruitX/apple/__init__.py:8:0: C0412: Imports from package fruitX are not grouped (ungrouped-imports)

----------------------------------------------------------------------
Your code has been rated at -10.00/10 (previous run: -18.33/10, +8.33)
# fruit2
************* Module apple
fruitX/apple/apple.py:5:0: E0401: Unable to import 'fruitX.banana.data' (import-error)
fruitX/apple/apple.py:8:0: E0401: Unable to import 'fruitX.apple.data' (import-error)
fruitX/apple/apple.py:6:0: C0411: third party import "import numpy" should be placed before "import fruitX.banana.data" (wrong-import-order)
fruitX/apple/apple.py:8:0: C0412: Imports from package fruitX are not grouped (ungrouped-imports)

---------------------------------------------------------------------
Your code has been rated at -10.00/10 (previous run: -1.67/10, -8.33)
# fruit3
************* Module apple
fruitX/apple/apple.py:6:0: C0411: third party import "import numpy" should be placed before "import fruitX.banana.data" (wrong-import-order)
fruitX/apple/apple.py:8:0: C0412: Imports from package fruitX are not grouped (ungrouped-imports)

---------------------------------------------------------------------
Your code has been rated at 6.67/10 (previous run: -10.00/10, +16.67)

Environment/config

the sort version is:


                 _                 _
                (_) ___  ___  _ __| |_
                | |/ _/ / _ \/ '__  _/
                | |\__ \/\_\/| |  | |_
                |_|\___/\___/\_/   \_/

      isort your imports, so you don't have to.

                    VERSION 5.10.0

the pylint version is:

pylint --version
pylint 2.11.1
astroid 2.8.4
Python 3.8.11 (default, Aug 16 2021, 12:04:33)

The rc diff applied on top of 2.11.1 generate-rc command is:

diff --git a/.pylint.rc b/.pylint.rc
index 6dfc542..547412c 100644
--- a/.pylint.rc
+++ b/.pylint.rc
@@ -85,6 +85,9 @@ disable=raw-checker-failed,
         suppressed-message,
         useless-suppression,
         deprecated-pragma,
+        unused-import,
+        missing-module-docstring,
+        invalid-name,
         use-symbolic-message-instead

 # Enable the message, report, category or checker with the given id(s). You can

@doublethefish
Copy link
Contributor

As an aside I came across this whilst looking for a pylint checker that checks for __init__.py files in packages because of the problems (like this one?) we experience when those files are missing in packages and namespaces, I am trying to understand when/if having such a checker is a good idea or not.

(And these issues don't just confuse newcomers to the language, either: they annoy many experienced developers as well.)

-- PEP-402, on issues like this one,

@jacobtylerwalls
Copy link
Member

Thanks @doublethefish, you've clearly demonstrated this is not currently an issue. I imagine this was fixed in isort itself some versions ago.

@jacobtylerwalls jacobtylerwalls closed this as not planned Won't fix, can't repro, duplicate, stale Jun 25, 2022
@jacobtylerwalls jacobtylerwalls added Upstream Bug 🪲 Bug in a dependency of pylint that is not astroid and removed Help wanted 🙏 Outside help would be appreciated, good for new contributors Good first issue Friendly and approachable by new contributors Hacktoberfest labels Jun 25, 2022
@lukasbindreiter
Copy link

Based on the investigation above from @doublethefish I did some more digging since for me the issue is still not resolved.

There is still an inconsistency between pylint and isort, however apparently it depends on the current working directory. The main issue seems to be by isort, however I did notice an inconsistent behaviour of pylint itself as well depending on where pylint is invoked.

Test setup

I used the fruits example from above and adapted it slightly to reproduce the issue. I also pushed a version to the fruits branch of my demo repository here

I have taken the fruit1 example from above, and adapted into a fruit2 version by moving the apple and banana subpackages to their own directory into two different packages, which are then installed with pip.
This results in the following directory structure:

.
├ .pylintrc  # disable=missing-module-docstring
├── apple   # provides fruit2.apple package, installed with pip install -e apple
│   ├── fruit2
│   │   └── apple
│   │       ├── __init__.py
│   │       └── data.py
│   └── setup.py
├── banana  # provides fruit2.banana package, installed with pip install -e banana
│   ├── fruit2
│   │   └── banana
│   │       ├── __init__.py
│   │       └── data.py
│   └── setup.py
└── fruit1
    ├── apple
    │   ├── __init__.py
    │   └── data.py
    └── banana
        ├── __init__.py
        └── data.py

And file contents as follows (same as above):

# fruit[1 or 2]/apple's __init__.py: 
import math  # std lib

import fruit[1 or 2].banana.data  # update the fruit number for each test candidate
import numpy

import fruit[1 or 2].apple.data
# fruit[1 or 2]/apple/data.py
data = "some apple data"
# fruit[1 or 2]/banana's __init__.py: 
# empty file
# fruit[1, 2 or 3]/banana/data.py
data = "some banana data"

pylint inconsistency

Now given the directory structure above, pylint behaves differently for me depending on where it is invoked:

> # in the root directory
> pylint --rcfile .pylintrc apple/fruit2/apple/__init__.py
************* Module apple
apple/fruit2/apple/__init__.py:6:0: C0412: Imports from package fruit2 are not grouped (ungrouped-imports)

------------------------------------------------------------------
Your code has been rated at 7.50/10 (previous run: 7.50/10, +0.00)




cd apple
> pylint --rcfile ../.pylintrc fruit2/apple/__init__.py

************* Module apple
fruit2/apple/__init__.py:4:0: C0411: third party import "import numpy" should be placed before "import fruit2.banana.data" (wrong-import-order)
fruit2/apple/__init__.py:6:0: C0412: Imports from package fruit2 are not grouped (ungrouped-imports)

------------------------------------------------------------------
Your code has been rated at 5.00/10 (previous run: 7.50/10, -2.50)

isort inconsistency

Invoking isort from the apple directory produces no changes:

cd apple
isort fruit2/apple/__init__.py

# no output, no changes

cd ..  # back to repo root
isort apple/fruit2/apple/__init__.py
> Fixing pylint-issue-demo/apple/fruit2/apple/__init__.py

with this diff:

diff --git a/apple/fruit2/apple/__init__.py b/apple/fruit2/apple/__init__.py
index fd23b38..1970b58 100644
--- a/apple/fruit2/apple/__init__.py
+++ b/apple/fruit2/apple/__init__.py
@@ -1,6 +1,5 @@
 import math

+import fruit2.apple.data
 import fruit2.banana.data
 import numpy
-
-import fruit2.apple.data

Note: For the fruit1 example the output is exactly as described by @doublethefish, pylint and isort behave consistent, and the produced diff by isort is this one:

diff --git a/fruit1/apple/__init__.py b/fruit1/apple/__init__.py
index fa74f83..642d85b 100644
--- a/fruit1/apple/__init__.py
+++ b/fruit1/apple/__init__.py
@@ -1,6 +1,6 @@
 import math

-import fruit1.banana.data
 import numpy

 import fruit1.apple.data
+import fruit1.banana.data

@jacobtylerwalls
Copy link
Member

Thanks for the additional details @lukasbindreiter! Have you tested against astroid main branch, by chance? There are lot of pending changes to namespace package handling waiting for a release.

@jacobtylerwalls jacobtylerwalls removed the Upstream Bug 🪲 Bug in a dependency of pylint that is not astroid label Jun 29, 2022
@Pierre-Sassoulas
Copy link
Member

pylint use isort under the hood. I think the inconsistencies are due to isort not detecting fruit1 or fruit2 as internal namespaces. It's not an easy problem to solve, and without stepping on the toe of Thimoty Crosley I think we can say that isort will not do that perfectly any time soon. In order to not have this problem you need to use a proper isort configuration with known_third_party and known_first_party defined (here adding fruit1 to know_first_party). I think we should close this issue as an upstream "bug" that we won't fix.

@doublethefish
Copy link
Contributor

doublethefish commented Jun 29, 2022

@lukasbindreiter having had a better look at your examples, what you have crafted in your example is a Nested Namespace Package, which will exhibit different behaviours depending on where you run isort/pylint - it cannot be otherwise.

Therer are two key differences:

  1. my examples are Regular Packages and Namespace Packages, and your example is a Nested Namespace Package (a slightly convoluted example of one, if you don't mind me saying so)
  2. in my examples we run the commands from inside each of the different fruitX directories (isolated self-contained packages, where there's no ambiguity between local, thirparty/site-packages and standard imports), whereas in your example we run the commands inside & outside of their equivalents. In the "outside" case isort and pylint cannot fully understand the import context, and so make assumptions.

Additionally, in your first cd apple (aka "inside) example, it is self-evident the fruit2.apple import is local, because the import path matches the cwd directory structure, and that fruit2.banana is not because it is NOT in the local path.

Then, in the second cd .. (aka "outside) example it is not self-evident, as there's not way to see the imported file in the cwd context, so it has to assume it's a third-party import. It would do that even if it was on PYTHONPATH (like in the PEP420 example for Nested Packages).

I guess one way to think about it is, "can we run the example code from cd ../'outside'?", The answer is no, not without extending PYTHONPATH. The same applies to the cd apple/ example, which won't be able to find the banana module without PYTHONPATH changes, but at least it is obvious in that case that the banana is third-party to that context.

Another way to think about it is run-context, or cwd context. Things in the cwd context will always be considered local, things outside (site-packages/ and PYTHONPATH) will be considered third-party, and standard libraries will be considered standard.

To summarise, depending on where we run isort/pylint Nested Namespace Packages will either be considered local or third-party. It is up to us to be consistent in how we run and use those tools. In such cases we get the most consistency running the tools "outside" any of the Nested Namespace Package's paths.

You might find PEP420 and importlib docs useful.

@doublethefish
Copy link
Contributor

I have changed my mind and OP is correct (now that I understand it). Imports inside a Nested Namespace Package should be treated consistently in all cases.

will exhibit different behaviours depending on where you run isort/pylint - it cannot be otherwise

I was wrong that it "cannot be otherwise" because I had the wrong logical basis, but it is hard for tools to detect local vs third-party imports in both types of Namespace Packages (those packages without __init__.py files marking a directory as package explicitly).

There are two logical options here that shape the design:

  1. Submodules of Nested Namespace Packages should be treated as being at the same level (the interpreter effectively merges them for us)
  2. Nested Namespace Packages should differentiate between local-submodules (local-sibling submdoules) and remote-submodules (non-local sibling submodules) in the same Namespace (which might be more logical and readable to developers)

As OP implies, option 2 is probably a better, more intuitive option.

Taking option 2 as correct then each sub-package in a Nested Namespace Package, should always differentiate between first-order imports (local-sibling submdoules) vs third-party imports (non-local sibling submodules), as follows (should be testable @yushao2):

# Namespace Package (currently ok), here for reference:
# NOTE: no __init__.py files although a regular package behaves the same.
src/
    parent/
        child_a/
            a.py  # should treat parent.child_a.a_sibling and parent.child_b.b_sibling as first-party (because both are inside parent)
            a_sibling.py
        child_b/
            b.py # should similarly treat a_sibing & b_sibling as first-party because of the shared root
            b_sibling.py

# Nested Namespace Package:
container/
    project1/
       src/
            parent/
                child/
                    a.py  # should treat parent.child.a_sibling as first-party, but parent.child.b_sibling as third-party (because of the project1 vs project2 root dirs)
                    a_sibling.py
    project2/
        src/
            parent/
                 child/
                    b.py # should similarly treat parent.child.b_sibling as first-party, but parent.child.a_sibling as third-party
                    b_sibling.py

To get to this conclusion, I wrote an issue (unsubmitted) for isort and reviewed one of my own NNPs. Doing so I realised that my NNP imports were actually incorrect vs what I hoped for at the time - I remember being frustrated at the time but found a way to adhere to what the tooling does, and accepted that. All of that is why I submitted #7085 to pylint, because it helps work around related issues.

I also looked at Tim's line in import this "Namespaces are one honking great idea -- let's do more of those!" which has really got me thinking.

So, yeah, OP is right after all :D

@doublethefish
Copy link
Contributor

doublethefish commented Jun 30, 2022

Also see this issue in isort: PyCQA/isort#1704

@Pierre-Sassoulas
Copy link
Member

@doublethefish do we agree it's an isort issue and pylint is not using isort incorrectly ?

@doublethefish
Copy link
Contributor

I don't know.

isort certainly has an issue, and I would assume that any upstream fix in isort would then have to be reflected in the corresponding pylint checker.

@Pierre-Sassoulas
Copy link
Member

@lukasbindreiter
Copy link

Thanks for the additional details @lukasbindreiter! Have you tested against astroid main branch, by chance? There are lot of pending changes to namespace package handling waiting for a release.

Just tested it with the latest astroid commit, but the behaviour is exactly the same as described in my last comment, so no changes there.

@lukasbindreiter
Copy link

@doublethefish Thanks for the detailed response. I was unaware of the term Nested Namespace Packages but that's exactly what we are dealing with. The reason for this setup is that the actual packages (apple and banana in this example) may be installed into different environments, where sometimes only apple is required, sometimes only banana or sometimes both.

From your response I agree that option 2 would be the most logical, intuitive approach, but I understand that it may very well be complicated for tools to achieve 😄

The issue with different behaviour from different working directories is not really that much of a problem, I just wanted to raise that as a concern as well, but the working directory itself can be easily enough configured in a CI pipeline.

The main problem (and also the reason for this issue in the first place) is this:

When invoked from the correct working dir, isort sorts fruit2/apple/__init__.py like this:
(Recognizing fruit2.banana as third-party and fruit2.apple as first-party)

import math

import fruit2.banana.data
import numpy

import fruit2.apple.data

However, when pylint checks this file it reports the following:

************* Module apple
apple/fruit2/apple/__init__.py:6:0: C0412: Imports from package fruit2 are not grouped (ungrouped-imports)

------------------------------------------------------------------
Your code has been rated at 7.50/10 (previous run: 7.50/10, +0.00)

I'm assuming this is because pylint considers both fruit2.apple and fruit2.banana as the same fruit2 package, therefore requiring them to be grouped.

For isort it is possible to configure known_first_party and known_third_party packages, but I think pylint ignores that isort configuration.
The only workaround I've found so far is to configure isort with known_first_party = ["fruit2"] which will result in this sort-order:

import math

import numpy

import fruit2.apple.data
import fruit2.banana.data

which is accepted by pylint without warnings but not really ideal in my opinion.

Any ideas for a workaround here are very welcome 😄

@Pierre-Sassoulas
Copy link
Member

but I think pylint ignores that isort configuration.

Thank you for analysis the issue in depth. We need to fix that part then. I think it needs some investigation, but in general pylint do not handle not being called from the root directory very well. Maybe we have to permit to define the path to the isort config in our own configuration.

@Pierre-Sassoulas Pierre-Sassoulas removed this from the 2.15.0 milestone Aug 25, 2022
@Pierre-Sassoulas Pierre-Sassoulas added the Needs PR This issue is accepted, sufficiently specified and now needs an implementation label Aug 25, 2022
@Pierre-Sassoulas Pierre-Sassoulas changed the title Incorrect Classifcation of Namespace Imports as First Party Pylint takes the isort configuration into account only if it's directly available in its launch directory Aug 25, 2022
@Pierre-Sassoulas Pierre-Sassoulas added Enhancement ✨ Improvement to a component and removed Bug 🪲 namespace-package labels Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
Development

No branches or pull requests

6 participants