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

chore: add isort #1666

Merged
merged 3 commits into from
Sep 24, 2022
Merged

chore: add isort #1666

merged 3 commits into from
Sep 24, 2022

Conversation

henryiii
Copy link
Member

@henryiii henryiii commented Sep 5, 2022

This adds isort, which sorts the includes, removing that as a variable and enabling things like injecting __future__ imports later.

If there are any "this needs to be imported before that" rules, we can add those as isort rules.

@henryiii
Copy link
Member Author

henryiii commented Sep 5, 2022

partially initialized module 'awkward' has no attribute 'nplike' Does anyone know off the top of their head what ordering is required to fix that? I can add a rule. (Imports should not be order dependent ideally. This might even mean there's a side effect when importing which would break if PEP 690 is accepted & used).

@agoose77
Copy link
Collaborator

agoose77 commented Sep 5, 2022

Currently I think our imports are somewhat order-dependent. The awkward.nplike module would need to go at the top (it's fairly independent, and any uses of ak inside the module body occur within function definitions. However, moving import awkward.nplike to the top of the module body raises new order-specific exceptions.

@jpivarski
Copy link
Member

The imports in awkward/_v2/__init__.py (and v1) are order-dependent: some class definitions use others (either by inheritance or by accessing constants), and the __init__.py file makes a set of them available at module-level (so ak.Array instead of ak.highlevel.Array) by importing them. It should be possible to sort any file in the codebase except awkward/__init__.py and awkward/_v2/__init__.py.

@codecov
Copy link

codecov bot commented Sep 6, 2022

Codecov Report

Merging #1666 (e6780ec) into main (e692946) will increase coverage by 1.01%.
The diff coverage is n/a.

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/__init__.py 96.87% <ø> (ø)
src/awkward/_broadcasting.py 93.41% <ø> (ø)
src/awkward/_connect/avro.py 87.17% <ø> (ø)
src/awkward/_connect/cling.py 24.90% <ø> (ø)
src/awkward/_connect/cuda/__init__.py 0.00% <ø> (ø)
src/awkward/_connect/jax/__init__.py 90.47% <ø> (ø)
src/awkward/_connect/jax/_reducers.py 76.92% <ø> (ø)
src/awkward/_connect/numba/arrayview.py 97.77% <ø> (ø)
src/awkward/_connect/numba/builder.py 81.60% <ø> (ø)
src/awkward/_connect/numba/layout.py 84.87% <ø> (ø)
... and 311 more

@henryiii henryiii marked this pull request as ready for review September 8, 2022 15:52
@jpivarski
Copy link
Member

As we discussed today, this will go after the 1.10.0 release and before the git-split.

@jpivarski
Copy link
Member

It looks like all of these changes are generated by this one change to .pre-commit-config.yaml:

@@ -19,6 +19,12 @@ repos:
  - id: requirements-txt-fixer
  - id: trailing-whitespace

- repo: https://github.com/PyCQA/isort
  rev: "5.10.1"
  hooks:
    - id: isort
      exclude: ^src/(awkward|awkward/_v2)/__init__\.py$

- repo: https://github.com/asottile/setup-cfg-fmt
  rev: v2.0.0
  hooks:

Considering that PR #1690 (dropping v1) is at an advanced stage, let's close this PR and reapply the above to main and main-v1 after #1690 has been merged into main.

@agoose77
Copy link
Collaborator

@jpivarski could I get your sign-off that we can merge this now that I've rebased it?

Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

This seems to be correct—it's changing the import order of post-v1-dropping files, such as src/awkward/_broadcasting.py (which didn't exist pre-v1-drop).

I'm signing off on it, but I'll let you merge it, in case that needs to be done in a special way.

@jpivarski
Copy link
Member

It's the same error in Codecov as before:

##[debug]Evaluating condition for step: 'Codecov'
##[debug]Evaluating: (success() && (matrix.python-version == '3.9'))
##[debug]Evaluating And:
##[debug]..Evaluating success:
##[debug]..=> true
##[debug]..Evaluating Equal:
##[debug]....Evaluating Index:
##[debug]......Evaluating matrix:
##[debug]......=> Object
##[debug]......Evaluating String:
##[debug]......=> 'python-version'
##[debug]....=> '3.9'
##[debug]....Evaluating String:
##[debug]....=> '3.9'
##[debug]..=> true
##[debug]=> true
##[debug]Expanded: (true && ('3.9' == '3.9'))
##[debug]Result: true
##[debug]Starting: Codecov
##[debug]Loading inputs
##[debug]Loading env
Run bash <(curl -s https://codecov.io/bash)
  bash <(curl -s https://codecov.io/bash)
  shell: /usr/bin/bash -e {0}
  env:
    PIP_ONLY_BINARY: cmake
    NUMPY_VERSION: 
    pythonLocation: /opt/hostedtoolcache/Python/3.9.13/x64
    PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.9.13/x64/lib/pkgconfig
    Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.9.13/x64
    Python[2](https://github.com/scikit-hep/awkward/actions/runs/3118432456/jobs/5057848983#step:15:2)_ROOT_DIR: /opt/hostedtoolcache/Python/[3](https://github.com/scikit-hep/awkward/actions/runs/3118432456/jobs/5057848983#step:15:3).9.13/x6[4](https://github.com/scikit-hep/awkward/actions/runs/3118432456/jobs/5057848983#step:15:4)
    Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.9.13/x64
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.9.13/x64/lib
##[debug]/usr/bin/bash -e /home/runner/work/_temp/d383494f-d68c-4116-96fd-a0760d6[5](https://github.com/scikit-hep/awkward/actions/runs/3118432456/jobs/5057848983#step:15:5)5e[6](https://github.com/scikit-hep/awkward/actions/runs/3118432456/jobs/5057848983#step:15:6)[7](https://github.com/scikit-hep/awkward/actions/runs/3118432456/jobs/5057848983#step:15:7).sh
/dev/fd/63: line 1: no: command not found
Error: Process completed with exit code [12](https://github.com/scikit-hep/awkward/actions/runs/3118432456/jobs/5057848983#step:15:12)7.
##[debug]Finishing: Codecov

On the other one, #1715, restarting the one job didn't fix it (not intermittent).

@agoose77 agoose77 merged commit 990e447 into main Sep 24, 2022
@agoose77 agoose77 deleted the henryiii/chore/isort branch September 24, 2022 21:07
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