[AIRFLOW-5362] Reorder imports #5944
Conversation
KevinYang21
force-pushed
the
kevin_yang_import_format
branch
11 times, most recently
from
3addf68 to
4421f97
Compare
KevinYang21
changed the title
KevinYang21
changed the title
|
@ashb PTAL, this should make it easier for your cherry-picking during release :D Owe this to you. |
|
What I'm not that sure is if/where I should put instruction about isort so that people can just sort their files in one command. CONTRIBUTING.md, CI output or somewhere else? |
|
Really nice @KevinYang21 ! I love it. It's a huge change though and it will have a lot of conflicts with #5786 so we have to think carefully which one to merge first (I believe it will be easier to have isort re-run after the pylint changes). I do not think we need explicitly state the instructions about isort in documentation as long as we add sorting script as another pre-commit check. We already have several pre-commit checks that modify the sources (for example the licence checks). Pre-commit works in the way that if it modifies any of the files while running - the whole pre-commit fails and it asks the user to add/commit the modified files. Also if someone does not use pre-commits (yet!) the checks fail in CI because some files were modified. Then in CI there is a very clear instruction "please run pre-commit and then add/commit changed files". I think it's better than comprehensive documentation because if travis fails you simply get instructions what to do :). And with properly implemented pre-commit check it will be super-easy to run by anyone even if they don't have pre-commits installed as git hooks. They would simply need to run 'pre-commit run sort--imports' if we add it with this id. I can help with adding it if needed :). With isort I think we do not even need docker image to run - you can simply specify isort as additional_dependencies as ['isort'] of python pre-commit hook and add isort configuration and specify the isort command to execute (see https://pre-commit.com/#pre-commit-configyaml---hooks) . In such case pre-commit will automatically create virtualenv, add isort as dependency and run the command specified (and if it won't modify any of the files it will be automatically "passed"). |
There was a problem hiding this comment.
Awesome @KevinYang21!
As Jarek mentions it's probably easier to merge the other PR first as all (most?) of these changes are automatic.
airflow/_vendor/nvd3/__init__.py
Outdated
| @@ -16,14 +16,14 @@ | |||
| 'scatterChart', 'discreteBarChart', 'multiBarChart'] | |||
|
|
|||
|
|
|||
| from . import ipynb | |||
| from .cumulativeLineChart import cumulativeLineChart | |||
There was a problem hiding this comment.
Can you exclude anything under airflow/_vendor from changes please?
Perhaps skip=airflow/_vendor in setup.cfg might do the trick? Should do given skip = build,.tox,venv is used in some of isort's examples.
There was a problem hiding this comment.
I have another thought.
I think one big change to order all imports in one go is not a good idea. We should probably use the same approach as we did for pylint.
Since we can have all of this as pre-commit hooks - we could do filtering in the same way we do for pylint pre-commit hooks/ci_scripts using the very same "pylint_todo.txt" file.
I think those two are fairly connected and we should use the list to only introduce such changes when we remove files from the pylint_todo.txt. And this way we make sure it will be gradually introduced.
Once we add it as pre-commit hook, it will have the property that someone fixing pylint will also have to make sure that imports are sorted. I also think about adding pydocstyle in exactly the same way -> so make sure the style is defined and checked for all files that are not on the pylint_todo.txt. This way reviewers will not have to even check if the documentation style we have is good and follows our agreed conventions. I think this should be automated rather than taking precious time of reviewers :).
What do you think (@KevinYang21 @BasPH @ashb @mik-laj ) ? I can add it quickly while I will be adding pydocstyle (I plan to test it while fixing @mik-laj comments for #5786) and I can add isort in very similar way there.
There was a problem hiding this comment.
I think that this change should be combined with automatic code formatting. Some automatic formatting tools also sort imports. If we now introduce import sorting, we will limit the choice of a tool that will be compatible, or we will have to re-sort all imports this time in a manner compatible with another tool.
There was a problem hiding this comment.
I think we should treat pylint + pydoc + import sort differently than whole source file formatting change.
Black formatting (or whatever we choose) changes whole files. Introducing it will basically rewrite ALL the lines of code we have. It will make it next to impossible to merge in-progress PRs (and in some cases those changes will be very difficult to resolve conflicts in those rebases).
On the other hand - sorting imports/adding pylint changes/updating documentation is a very small and localised change to a small part of the code - even if it is done in all files.
So changing import orders or adding docs is far less invasive than formatting change (you can still easily merge other changes after you applied them).
That's why I think it's worth to sort imports now separately. Even if formatting tool will change the sort order later, it will be done together with plenty of other changes for those files, so this will be pretty much no-issue if also sorting order for imports change then.
There was a problem hiding this comment.
This change shouldn't affect all PRs, only ones that new imports.
With the git blame --ignore-revs-file option (I thought there was a default way to set that but it's a client only setting) I would prefer changes like this to be a single commit rather than multiple.
There was a problem hiding this comment.
@ashb - If you refer to import ordering I think import sort is rather non-invasive so I do not think it falls into the 'git blame' case. It is rather localised and does not really affect the "meat" of the source code. But If you refer to reformatting - I agree that re-formatting the code if we decide to do it should be done as single big change and one that we can exclude with --ignore-revs-file.
There was a problem hiding this comment.
BTW. Some goodies from Chromium team: git hyper-blame command - one that can ignore commits stored in committed .git-blame-ignore-revs (https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-hyper-blame.html)
:D
There was a problem hiding this comment.
Will exclude unnecessary packages. Given how automatic isort can be esp. with pre-commit, I prefer to roll the change out in one commit.
|
@potiuk The other PR can be merged first for sure--beauty of isort is that it comes with a sorting engine so I can redo this PR in a couple mins. And the pre-commit approach sounds good, I'll take care of it. |
KevinYang21
force-pushed
the
kevin_yang_import_format
branch
4 times, most recently
from
f680276 to
b0f68ff
Compare
|
OK. Sure. Let me merge my PR and you can redo yours afterwards. I am ok with one single commit for everything as well as linking it with pylint changes. Both work for me. |
KevinYang21
force-pushed
the
kevin_yang_import_format
branch
from
b0f68ff to
1b243d7
Compare
.pre-commit-config.yaml
Outdated
| - id: isort | ||
| name: Run isort to sort imports | ||
| language: python | ||
| entry: isort -rc . |
There was a problem hiding this comment.
Remove -rc . please. Pre-commit by default passes changed filenames as command arguments (default configuration is pass_filenames: true).
This has a number of advantages:
- in regular pre-commit it will only pass the names of files that changed/were added in this commit/series of commit when pushing (this makes it much faster for smaller commits)
- when you have big number of changes it will automatically split the command to several commands run in parallel (equal to the number of threads you have available on your PC) - distributing the changed files between those. If you have 4 CPUs with 2 threads each it will split the big list of files between 8 isort commands and run them in parallel. This means that if you remove all your changes now and run
pre-commit run --all-filesit will run 8 times faster thanisort -rc ..
There was a problem hiding this comment.
roger that 👍 I was under the impression that happens only after u set the files but obviously I should go read doc first :P
KevinYang21
force-pushed
the
kevin_yang_import_format
branch
4 times, most recently
from
9c45828 to
b389e83
Compare
|
@potiuk PTAL, will rebase right before merge. |
KevinYang21
force-pushed
the
kevin_yang_import_format
branch
from
b389e83 to
049538a
Compare
| from typing import Optional, Callable | ||
| from airflow import version | ||
| from airflow.utils.log.logging_mixin import LoggingMixin | ||
| isort:skip_file |
There was a problem hiding this comment.
Does this now show up in the API section of the rendered docs?
Is there a reason we are skipping this file but making changes to it? Did perhaps we need a blank line first?
| isort:skip_file | |
| isort:skip_file |
There was a problem hiding this comment.
There're a lot implied dependencies inside multiple imports in this file. I tried a couple times try to reorder a couple but it keeps getting me more errors so I decided to skip it with the maximum changes I tried that won't break it so I don't have to add expection comment in multiple lines. And I'll for sure add that blank line :P
There was a problem hiding this comment.
And sry I'm missing the docs part. Do you mind help pointing me to this rendered docs you referred to please?
There was a problem hiding this comment.
BTW. I just merged the pylint changes few days ago - so rebasing this one and re-running the isort should be good to go @KevinYang21
|
https://pypi.org/project/zimports/ might help with making this automatic as part of the pre-commit hook. |
KevinYang21
force-pushed
the
kevin_yang_import_format
branch
from
049538a to
7b44ff3
Compare
KevinYang21
force-pushed
the
kevin_yang_import_format
branch
from
7b44ff3 to
1a85aa9
Compare
Codecov Report
@@ Coverage Diff @@
## master #5944 +/- ##
==========================================
- Coverage 80.06% 80.04% -0.02%
==========================================
Files 610 610
Lines 35286 35261 -25
==========================================
- Hits 28251 28226 -25
Misses 7035 7035
Continue to review full report at Codecov.
|
|
@ashb Ya if we use @potiuk Rebased again, mind take a quick pass before I merge it? Thank you. |
|
Thanks @KevinYang21 -> I merged and cherry-picked (I rerun isort) to v1-10-test. This way it will be easier to cherry-pick changes. |
|
Turned out that sorting v1-10-test causes some circular imports which are rather difficult to fix without splitting some of the files as it was done in 2.0.0 so I reverted it back in v1-10-test :( |
|
@kaxil we have an internal pipeline that pulls latest commit from master for deployment purpose. Whenever there is a force update on the master, it invalidates our internal tree, which requires human intervention to get it unblocked. |
|
It was my fault @houqp (sorry @kaxil you had to do that). I mistakenly rebased a change instead of squashing and Kaxil had to correct my mistake. Apologies ! @kaxil - maybe we can simply completely disable rebasing option in GitHub ? I sometimes switch to it and GitHub remembers the last setting unfortunately :( |
(cherry picked from commit d719e1f)
(cherry picked from commit d719e1f)
Make sure you have checked all steps below.
Jira
Description
Imports in the projects are not ordered properly and there's no unified way to sort them. This causes big headaches resolving conflicts in rebasing/cherry-picking and the lookings of the code base. What're included in the PR:
Tests
No code change, just changing imports.
Commits
Documentation
Code Quality
flake8