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

Make isort venv, or other environment, independent #1147

Closed
timothycrosley opened this issue Feb 17, 2020 · 5 comments
Closed

Make isort venv, or other environment, independent #1147

timothycrosley opened this issue Feb 17, 2020 · 5 comments
Labels
enhancement New feature or request
Milestone

Comments

@timothycrosley
Copy link
Member

timothycrosley commented Feb 17, 2020

This ticket represents the work to make isort work the same, by default independent to what environments are loaded or installed. This is increasingly important as isort is being utilized widely within CI/CD systems, where the likely hood that a build system will have a different venv than a developers machine becomes very high.

Suggested approach:

Default finding should do the following on a per import basis:

  • CUSTOM: import name exists in any custom specified import sections.
  • FUTURE: import starts with from __future__
  • STDLIB: import name exists in comprehensive built in definition of stdlib imports
  • FIRSTPARTY: import name starts with .
  • FIRSTPARTY: import code exists within the files isort is told to sort.
  • FIRSTPARTY: import code exists in same directory as any files isort is told to sort.
  • THIRDPARTY: Everything else.

In this way, by default isort will not use any magic and will be fully repeatable 🤞. The older approach should be made available via a flag, such as --environment-identifcation-magic and corresponding config option. No optional dependencies (such as toml, setup.py requirements etc) should be used by default, as using these optional dependencies by default is a recipe for non-repeatable runs and user confusion.

Related:
#1058
#1071
#1104
#1098
#1048
#1007
#952
#910
And more that have been closed from previous magic improvement work, though this is the first magic removal attempt.

@ndevenish
Copy link
Contributor

I just discovered this testing the master branch on our code sorted with 4.3.21 with everything mixing into first-party (within isolated pre-commit hooks is out direct use case here). Good to see it's known.

The change to third-by-default sounds sane; for our use case I'd be happy with that and explicitly specifying first-party, though appreciate some level of heuristic magic might make it more flexible.

Doing so based on where it's run sounds to me like maybe a risk of getting different behaviour based on where it's run from (e.g. along with pre-commit we have weekly jobs that run inside and over whole multi-project trees, in case people haven't been installing the hooks....). Perhaps "FIRSTPARTY is anything above in the current package"?, though I imagine namespace packages make this hard to determine heuristically also.

From these rules It looks like local/relative (.) imports are proposed to be counted as FIRSTPARTY - these are currently LOCAL, I believe? Planning to remove this distinction, or just not specifying here?

@asottile
Copy link
Member

asottile commented Mar 9, 2020

fwiw, you may find some prior art in https://github.com/asottile/aspy.refactor_imports (which powers reorder-python-imports) -- notably you'll likely run into the issue of src layouts where you'll want some sort of argument to indicate where the source roots are -- the tool above uses --application-directories

as a workaround, and what's been suggested for usage with pre-commit at least, is to utilize seed-isort-config which leverages aspy.refactor_imports to populate known_third_party and then isort "just works"

I suggested something similar way back in #184 ;)

musoke added a commit to auckland-cosmo/LearnAsYouGoEmulator that referenced this issue Mar 9, 2020
- Configure github actions to run pre-commit on the repo to check
style, etc.
- Update to stable/new versions of hooks to avoid spurious failures from
upstream updates.
- Flag a few imports in tests and examples with NOQA.  This is to avoid
bugs in isort.  In particular, isort is environment dependent.

    PyCQA/isort#1147
    PyCQA/isort#952
    PyCQA/isort#1007

- Disable one flake8 lint: 501, line too long
musoke added a commit to auckland-cosmo/LearnAsYouGoEmulator that referenced this issue Mar 9, 2020
- Configure github actions to run pre-commit on the repo to check
style, etc.

- Update to stable/new versions of hooks to avoid spurious failures from
upstream updates.

- Flag a few imports in tests and examples with NOQA.  This is to avoid
bugs in isort.  In particular, isort is environment dependent.

    PyCQA/isort#1147
    PyCQA/isort#952
    PyCQA/isort#1007

- Disable one flake8 lint: 501, line too long
musoke added a commit to auckland-cosmo/LearnAsYouGoEmulator that referenced this issue Mar 9, 2020
- Configure github actions to run pre-commit on the repo to check
style, etc.

- Update to stable/new versions of hooks to avoid spurious failures from
upstream updates.

- Flag a few imports in tests and examples with NOQA.  This is to avoid
bugs in isort.  In particular, isort is environment dependent.

    PyCQA/isort#1147
    PyCQA/isort#952
    PyCQA/isort#1007

- Disable one flake8 lint: 501, line too long
@beneisner
Copy link

Is there any interest in/plan for allowing the [tool.poetry.dependencies] and [tool.poetry.dev-dependencies] to seed the isort THIRDPARTY list?

@saroad2
Copy link

saroad2 commented May 16, 2020

When does this fix is about to be merged? This is making a lot of false-positive errors in my CI/CD process.

@timothycrosley
Copy link
Member Author

This is now merged into development branch and ready for release with 5.0.0. @beneisner, no - that's the exact kind of magic I'm aiming to remove. The good news, is with this latest version you shouldn't need it.

StanczakDominik added a commit to StanczakDominik/PlasmaPy that referenced this issue Jun 20, 2020
This temporarily sets isort to commit
5a9624dd8105bb60322a095efb1a5df18322fe39
on GitHub, which is the current most recent
version - the pypi version has not been released yet.

This change is directly caused by
PyCQA/isort#1147 (comment)

isort was removed previously as it did not work consistently
in CI.
stuhood added a commit to pantsbuild/pants that referenced this issue Jul 27, 2020
This reverts commit c827416.

Between isort 4 and isort 5, the requirement on which files are present in the sandbox has changed, and this can result in unstable sort orders. See PyCQA/isort#1147 for the likely cause. Under isort 5, the following set of roots (via `./pants --changed-parent=master list` can result in an order that does not align with `./pants list ::`: https://gist.github.com/stuhood/ec04e86b8a1f44d8b11dcb79a1732a8f

In order to use isort 5 stably in a sandbox, we will need to include the necessary transitive dependencies for it to execute its new first-party-detection algorithm, or disable it.

[ci skip-rust-tests]
OneDirection9 added a commit to OneDirection9/foundation that referenced this issue Jul 30, 2020
According to PyCQA/isort#1147, import code exists in same directory as any files isort is told to sort will be recognized as FIRSTPARTY

Signed-off-by: Zhipeng Han <hanzhipeng9@gmail.com>
OneDirection9 added a commit to OneDirection9/foundation that referenced this issue Jul 30, 2020
According to PyCQA/isort#1147, import code exists in same directory as any files isort is told to sort will be recognized as FIRSTPARTY

Signed-off-by: Zhipeng Han <hanzhipeng9@gmail.com>
ndevenish added a commit to cctbx/dxtbx that referenced this issue Aug 5, 2020
This mostly runs on commit now - occasionally either people will commit
without a full precommit environment, or encounter an ordering bug in
isort - PyCQA/isort#1147 (which
should be fixed in the next isort release)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants