isort everything

[yadudoc]

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

[benclifford]

Can you make this enforced in CI?

[benclifford]

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]

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]

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]

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.

[benclifford]

this is not part of parsl?

[benclifford]

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]

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.