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

isort everything #3421

Merged
merged 3 commits into from
Jun 3, 2024
Merged

isort everything #3421

merged 3 commits into from
Jun 3, 2024

Conversation

yadudoc
Copy link
Member

@yadudoc yadudoc commented May 8, 2024

Description

This PR should not make changes beyond sorting the imports. Now the changed import order can cause problems.
I'm adding an .isort.cfg file and updating the test-requirements.txt. I will add it to test actions and add a pre-commit config separately

Type of change

Choose which options apply, and delete the ones which do not apply.

  • Code maintenance/cleanup

@yadudoc yadudoc requested review from benclifford and rjmello May 8, 2024 16:41
@yadudoc yadudoc marked this pull request as draft May 8, 2024 16:45
@benclifford
Copy link
Collaborator

Can you make this enforced in CI?

@benclifford
Copy link
Collaborator

By enforced, I mean, fail in CI if stuff is not sorted - I don't mean "make bot commits to someone's PR branch".

This is a big change to code style and stuff will fall out of order after one pull request if not enforced - see #3106 (review) on previous work PR

@benclifford
Copy link
Collaborator

I can see CI is failing with circular import problems:

 E   ImportError: cannot import name 'TorqueProvider' from partially initialized module 'parsl.providers' (most likely due to a circular import) (/home/runner/work/parsl/parsl/parsl/providers/__init__.py)

I encountered this sort of stuff working on adding type checking a lot, and mostly the resolution was to use

import whatever as w
...
f(w.Foo)

in place of what previously had been:

from whatever import Foo
...
f(Foo)

because that first import syntax does not require an import to be completed, so is more tolerant of circular imports.

@Andrew-S-Rosen
Copy link
Contributor

Andrew-S-Rosen commented May 9, 2024

By enforced, I mean, fail in CI if stuff is not sorted - I don't mean "make bot commits to someone's PR branch".

What is the concern with having something like the pre-commit CI bot enabled so it can auto-fix and not become a hindrance for contributors? It works quite seamlessly in my experience.

@yadudoc yadudoc force-pushed the isort_everything branch 2 times, most recently from ed52660 to a9bed98 Compare May 9, 2024 20:26
@yadudoc yadudoc marked this pull request as ready for review May 10, 2024 18:35
@yadudoc
Copy link
Member Author

yadudoc commented May 10, 2024

I've cleaned up the PR and merged a fix for the circular import deps (#3424).
@benclifford I've updated the Makefile (and make test) to test with isort to fail in CI if the imports are not clean.
@Andrew-S-Rosen, I think it is a matter of consistency. We've followed this model so far where devs are responsible for pushing clean code and CI is responsible for running tests and checks, this PR just sticks to what we've done so far.

.isort.cfg Outdated
@@ -0,0 +1,3 @@
[settings]
profile = black
known_first_party = globus-compute-sdk, globus-compute-endpoint
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not part of parsl?

@benclifford
Copy link
Collaborator

What is the concern with having something like the pre-commit CI bot enabled so it can auto-fix and not become a hindrance for contributors? It works quite seamlessly in my experience.

@Andrew-S-Rosen to address this a bit more - I have a massive personal dislike for stuff that messes with someones branches: this turns "you made a push; then you made a fix; then you made a push again" into "your push doesn't work" which for the very junior developers that often contribute to parsl, that turns into "now it's my problem to teach you about git and how we robo-sabotaged your branch for you!".

More experienced developers are quite capable of running this code (automatically if and when they want that) in their own dev environment, without needing the CI to be involved other than checking they did it right - although there are plenty of use cases where this doesn't matter (for example, any dev/demo branch that isn't immediately intended to be merged to master)

@Andrew-S-Rosen
Copy link
Contributor

Andrew-S-Rosen commented May 15, 2024

Interesting. It has been my personal experience from working on other OSS projects that the exact opposite happens. When the CI gatekeeps based on lint checks, contributors get frustrated because they don't know or understand how to deal with pre-commit hooks and then try manually to fix the lint errors (with constant X's delaying progress). Ultimately, this leaves many PRs abandoned for silly reasons. When lint fixes are pushed automatically by a bot, there is less frustration (I have not seen contributors reporting any issues with just pulling the new changes). I help maintain a code pymatgen where there is a bot and a code ASE where there is not, and it's like night and day.

But personally, I don't care much. Just sharing my own experiences.

@yadudoc yadudoc force-pushed the isort_everything branch from acef0b9 to 7acf67b Compare June 3, 2024 17:10
@benclifford
Copy link
Collaborator

ok, I validated this PR like this:

benc@parsl-dev-3-12-8060:~/parsl/src/parsl$ git checkout 8bb9f4d5590dd4595829cd6eea2996a47a4cc5dd
HEAD is now at 8bb9f4d559 Adding isort config, entries to Makefile to run checks along with CI, and test requirements

benc@parsl-dev-3-12-8060:~/parsl/src/parsl$ isort parsl docs setup.py 
Fixing [... many filenames...]

benc@parsl-dev-3-12-8060:~/parsl/src/parsl$ git diff -r b90d5f3eda949896b6c7031ed8c6d7903adc4a39

and checked that that diff statement didn't output any difference

@benclifford benclifford merged commit 4934c30 into master Jun 3, 2024
6 checks passed
@benclifford benclifford deleted the isort_everything branch June 3, 2024 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants