Skip to content

imports: sort imports everywhere in Spack#24695

Merged
adamjstewart merged 4 commits intodevelopfrom
fix-all-imports
Jul 8, 2021
Merged

imports: sort imports everywhere in Spack#24695
adamjstewart merged 4 commits intodevelopfrom
fix-all-imports

Conversation

@tgamblin
Copy link
Copy Markdown
Member

@tgamblin tgamblin commented Jul 4, 2021

We enabled import order checking in #23947, but fixing things manually drives people crazy. This used spack style --fix --all from #24071 to automatically sort everything in Spack so PR submitters won't have to deal with it.

This should go in after #24071, as it assumes we're using isort, not flake8-import-order to order things. isort seems to be more flexible and allows llnl mports to be in their own group before spack ones, so this seems like a good switch.

On the plus side, this does make imports way more readable. I like being able to find them quickly at the tops of files.

@tgamblin tgamblin marked this pull request as draft July 4, 2021 09:44
@vsoch
Copy link
Copy Markdown
Member

vsoch commented Jul 4, 2021

I would probably tackle the list of issues here: https://github.com/spack/spack/pull/24695/checks?check_run_id=2982839061 before it's ready for review!

@tgamblin
Copy link
Copy Markdown
Member Author

tgamblin commented Jul 4, 2021 •
edited
Loading

Those are wrong b/c they use the flake-import-order rules — that needs to be re-run after #24071. Main difference is separate group for LLNL vs spack (which I guess flake8 didn’t support well).

#24071 switches to just one tool for imports — isort. Probably should’ve just based this on #24071.

@tgamblin tgamblin force-pushed the fix-all-imports branch 2 times, most recently from 4abf918 to c44845f Compare July 6, 2021 10:00
@adamjstewart adamjstewart mentioned this pull request Jul 6, 2021
Merged
@sethrj sethrj mentioned this pull request Jul 7, 2021
Merged
@adamjstewart
Copy link
Copy Markdown
Member

adamjstewart commented Jul 8, 2021

Is this ready to merge after a rebase?

@tgamblin tgamblin marked this pull request as ready for review July 8, 2021 00:38
@tgamblin
Copy link
Copy Markdown
Member Author

tgamblin commented Jul 8, 2021 •
edited
Loading

@adamjstewart yeah but let the tests to run -- there were import errors last time -- want to make sure they're not lurking.

adamjstewart
adamjstewart previously approved these changes Jul 8, 2021
View reviewed changes
@adamjstewart
Copy link
Copy Markdown
Member

adamjstewart commented Jul 8, 2021

Didn't actually review anything because GitHub can't load the changes, but I trust that isort didn't break much as long as the tests pass. I'll have to actually check out the black autoformatting branch to view that one as I'm more concerned about how it formats packages.

@adamjstewart adamjstewart enabled auto-merge (squash) July 8, 2021 02:09
@tgamblin
Copy link
Copy Markdown
Member Author

tgamblin commented Jul 8, 2021 •
edited
Loading

@adamjstewart: the mac failure is due to pep8-naming being installed on the Mac builder but not Linux, so we’ll remove it.

The other failure is a circular import that I can’t reproduce. @trws saw it on #24718 (comment) and couldn’t reproduce it… if you have ideas let us know. @alalazo and I are working on reproducing it.

@alalazo alalazo mentioned this pull request Jul 8, 2021
Closed
tgamblin added 4 commits July 8, 2021 13:29
@tgamblin
fix remaining flake8 errors
5639fd2
@tgamblin
imports: sort imports everywhere in Spack
ea82734 
We enabled import order checking in #23947, but fixing things manually drives
people crazy. This used `spack style --fix --all` from #24071 to automatically
sort everything in Spack so PR submitters won't have to deal with it.

This should go in after #24071, as it assumes we're using `isort`, not
`flake8-import-order` to order things. `isort` seems to be more flexible and
allows `llnl` mports to be in their own group before `spack` ones, so this
seems like a good switch.
@tgamblin
ci: Don't install pep8-naming checks for CI
443e21a 
The style checks in the macOS builds behaved slightly differently from the other builds
because we were installing the `pep8-naming` extension for some reason.

-[x] remove `pep8-naming` from macOS installs
@tgamblin
imports: fix circular import introduced by import sorting
fc22dfa
@tgamblin
Copy link
Copy Markdown
Member Author

tgamblin commented Jul 8, 2021

OK I think @alalazo and I have resolved the circular import thing -- it was not intuitive and it's not clear why this caused it.

@adamjstewart
Copy link
Copy Markdown
Member

adamjstewart commented Jul 8, 2021

What was the cause of the circular import? I saw the fix, but it isn't clear to me how that could cause problems.

P.S. We could enable https://pycqa.github.io/isort/docs/configuration/options.html#remove-redundant-aliases if we want to prevent it from happening again.

@trws
Copy link
Copy Markdown
Contributor

trws commented Jul 8, 2021

Oh wow, import spack got reordered with respect to the import of spack.compilers in some file so the __all__ that hides all the other modules under spack from access through the spack module started hiding compilers right? That's... That's really, really nasty. Why this fix rather than changing 2492 to reference the alias?

@tgamblin
Copy link
Copy Markdown
Member Author

tgamblin commented Jul 8, 2021

Just found it. It was this issue, which was fixed in Python 3.7:

TL; DR: before 3.7, there are issues with circular imports when using import as. See the discussion there for the details, but we may continue to hit these types of issues as long as we support <= 3.6.

@tgamblin
Copy link
Copy Markdown
Member Author

tgamblin commented Jul 8, 2021

Why this fix rather than changing 2492 to reference the alias?

I think where there's not a good reason to be terse it's plenty clear to just say spack.compilers... I could see us coming up with something else called compilers and shooting ourselves in the foot again, so I picked the one I thought would be more robust.

@tgamblin
Copy link
Copy Markdown
Member Author

tgamblin commented Jul 8, 2021

It seems like the only 100% safe thing to do in Python (i.e., the only thing resilient to adds, reorders, etc.) is going to be to import X.Y instead of import X.Y as Z. Which is annoying. I suppose the cause won't be quite as mysterious in the future as we won't be considering ALL reorderings at once, so this will mostly hash out in CI. I don't see a reason to put a hard and fast rule on this.

@trws
Copy link
Copy Markdown
Contributor

trws commented Jul 8, 2021

That makes sense. Nice detective work. 👍

@adamjstewart adamjstewart merged commit 24c01d5 into develop Jul 8, 2021
@adamjstewart adamjstewart deleted the fix-all-imports branch July 8, 2021 22:12
@tgamblin
Copy link
Copy Markdown
Member Author

tgamblin commented Jul 8, 2021 •
edited
Loading

Ok everyone can rejoice -- all the imports are fixed and you can your own with spack style --fix instead of fiddling with them.

@tgamblin
Copy link
Copy Markdown
Member Author

tgamblin commented Jul 8, 2021

@adamjstewart right now:

giphy

@adamjstewart
Copy link
Copy Markdown
Member

adamjstewart commented Jul 8, 2021

Now we just need to merge that black PR so we can break every single open PR in Spack 😄

@tgamblin tgamblin mentioned this pull request Jul 8, 2021
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adamjstewart adamjstewart adamjstewart approved these changes

@vsoch vsoch Awaiting requested review from vsoch

@alalazo alalazo Awaiting requested review from alalazo

@sethrj sethrj Awaiting requested review from sethrj

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tgamblin @vsoch @adamjstewart @trws