CI: Added isort import check for the code on pull-request #4242

nathanpainchaud · 2020-10-19T20:37:24Z

What does this PR do?

Before submitting

Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
Did you read the contributor guideline, Pull Request section?
Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
Did you make sure to update the documentation with your changes?
Did you write any new necessary tests?
Did you verify new and existing tests pass locally with your changes?
If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

setup.cfg

ydcjeff · 2020-10-20T07:07:04Z

pyproject.toml

+
+[tool.isort]
+known_first_party = ["pytorch_lightning","tests","pl_examples"]
+profile = "black"


@Borda would we want use black style for sorting or other? There is also pycharm style https://pycqa.github.io/isort/docs/configuration/profiles/

Hi @nathanpainchaud which profile do you think is suitable for lightning which follows pep8 for code formatting?

can we get some examples?
for me split to three sections: build-in, third-party and locals

This is with black profile using command as this PR

import collections import copy import inspect import os import re import tempfile from abc import ABC from argparse import Namespace from typing import Any, Callable, Dict, List, Optional, Sequence, Tuple, Union import torch from torch import ScriptModule, Tensor from torch.nn import Module from torch.optim.optimizer import Optimizer from pytorch_lightning import _logger as log from pytorch_lightning.core.grads import GradInformation from pytorch_lightning.core.hooks import CheckpointHooks, DataHooks, ModelHooks from pytorch_lightning.core.memory import ModelSummary from pytorch_lightning.core.saving import ALLOWED_CONFIG_TYPES, PRIMITIVE_TYPES, ModelIO from pytorch_lightning.core.step_result import Result from pytorch_lightning.utilities import rank_zero_warn from pytorch_lightning.utilities.device_dtype_mixin import DeviceDtypeModuleMixin from pytorch_lightning.utilities.exceptions import MisconfigurationException from pytorch_lightning.utilities.parsing import AttributeDict, collect_init_args, get_init_args from pytorch_lightning.utilities.xla_device_utils import XLADeviceUtils

with pycharm profile

import collections import copy import inspect import os import re import tempfile from abc import ABC from argparse import Namespace from typing import ( Any, Callable, Dict, List, Optional, Sequence, Tuple, Union ) import torch from torch import ( ScriptModule, Tensor ) from torch.nn import Module from torch.optim.optimizer import Optimizer from pytorch_lightning import _logger as log from pytorch_lightning.core.grads import GradInformation from pytorch_lightning.core.hooks import ( CheckpointHooks, DataHooks, ModelHooks ) from pytorch_lightning.core.memory import ModelSummary from pytorch_lightning.core.saving import ( ALLOWED_CONFIG_TYPES, PRIMITIVE_TYPES, ModelIO ) from pytorch_lightning.core.step_result import Result from pytorch_lightning.utilities import rank_zero_warn from pytorch_lightning.utilities.device_dtype_mixin import DeviceDtypeModuleMixin from pytorch_lightning.utilities.exceptions import MisconfigurationException from pytorch_lightning.utilities.parsing import ( AttributeDict, collect_init_args, get_init_args ) from pytorch_lightning.utilities.xla_device_utils import XLADeviceUtils

with google profile

from abc import ABC from argparse import Namespace import collections import copy import inspect import os import re import tempfile from typing import Any, Callable, Dict, List, Optional, Sequence, Tuple, Union import torch from torch import ScriptModule from torch import Tensor from torch.nn import Module from torch.optim.optimizer import Optimizer from pytorch_lightning import _logger as log from pytorch_lightning.core.grads import GradInformation from pytorch_lightning.core.hooks import CheckpointHooks from pytorch_lightning.core.hooks import DataHooks from pytorch_lightning.core.hooks import ModelHooks from pytorch_lightning.core.memory import ModelSummary from pytorch_lightning.core.saving import ALLOWED_CONFIG_TYPES from pytorch_lightning.core.saving import ModelIO from pytorch_lightning.core.saving import PRIMITIVE_TYPES from pytorch_lightning.core.step_result import Result from pytorch_lightning.utilities import rank_zero_warn from pytorch_lightning.utilities.device_dtype_mixin import DeviceDtypeModuleMixin from pytorch_lightning.utilities.exceptions import MisconfigurationException from pytorch_lightning.utilities.parsing import AttributeDict from pytorch_lightning.utilities.parsing import collect_init_args from pytorch_lightning.utilities.parsing import get_init_args from pytorch_lightning.utilities.xla_device_utils import XLADeviceUtils

no profile

import collections import copy import inspect import os import re import tempfile from abc import ABC from argparse import Namespace from typing import Any, Callable, Dict, List, Optional, Sequence, Tuple, Union import torch from torch import ScriptModule, Tensor from torch.nn import Module from torch.optim.optimizer import Optimizer from pytorch_lightning import _logger as log from pytorch_lightning.core.grads import GradInformation from pytorch_lightning.core.hooks import CheckpointHooks, DataHooks, ModelHooks from pytorch_lightning.core.memory import ModelSummary from pytorch_lightning.core.saving import ALLOWED_CONFIG_TYPES, PRIMITIVE_TYPES, ModelIO from pytorch_lightning.core.step_result import Result from pytorch_lightning.utilities import rank_zero_warn from pytorch_lightning.utilities.device_dtype_mixin import DeviceDtypeModuleMixin from pytorch_lightning.utilities.exceptions import MisconfigurationException from pytorch_lightning.utilities.parsing import AttributeDict, collect_init_args, get_init_args from pytorch_lightning.utilities.xla_device_utils import XLADeviceUtils

All of them split built-in, third-party, locals

@ydcjeff already gave a good explanation. What I can add to this is that since PL has already decided to use black for formatting the code, I would heavily suggest using the black profile. Otherwise, we might fall into a "deadlock" between checks, where isort and black suggest conflicting changes (speaking from experience there 😛) Using the black profile guarantees that both tools won't conflict with each other.

I think PL isn't using black
cc: @Borda

not back for codebase just for formating imports

Ah, my bad! I saw the pre-commit hook for black and I thought it was already in use. In that case, it's a bit up to personal preference, and what we want to prioritize.

I like the black format myself, but for a community project like PL I think a profile with single line imports (like google and open_stack) might prevent some conflicts in the imports.

+1 from me on black with consistent string formatting too. makes my OCD go crazy when i see some variables with single quotes and others with double quotes in the same file :)

akihironitta · 2020-10-20T07:52:15Z

.github/workflows/code-formatting.yml

+      - name: Install isort
+        run: pip install isort==5.6.4
+      - name: Run isort
+        run: isort --settings-path=./pyproject.toml --check-only .


Not a strong opinion, but how about adding --diff option?

-df, --diff Prints a diff of all the changes isort would make to a file, instead of changing it in place

Hey @nathanpainchaud, --diff would be nice to see actually.

The --check-only option already lists the file that would be changed, without changing them in-place. From what I understand, --diff would print all the changes, even inside each file. I've got no problem adding it, I just want to make sure it wouldn't be considered too noisy in the end.

tchaton

Thanks a lot ! Great addition to Pytorch Lightning. Would you mind adding it in pre-commit and resolving unit-tests ?

tchaton · 2020-10-20T13:38:32Z

.github/workflows/code-formatting.yml

+      - name: Install isort
+        run: pip install isort==5.6.4
+      - name: Run isort
+        run: isort --settings-path=./pyproject.toml --check-only .


Hey @nathanpainchaud, --diff would be nice to see actually.

tchaton · 2020-10-20T13:40:39Z

.github/workflows/code-formatting.yml

@@ -7,6 +7,21 @@ on:  # Trigger the workflow on push or pull request, but only for the master bra
    branches: [master]

 jobs:
+  isort:


Could you add isort to pre-commit too. Here is an example: https://github.com/PyCQA/isort/blob/develop/.pre-commit-config.yaml

I already did actually 😉 But I asked @Borda about it, and he wanted to keep the pre-commit hook for a different PR. When you're ready for it, you can just ping me though, because I already have it locally!

yes as with accepting the CI check we need to format the codebase accordingly...

nathanpainchaud · 2020-10-20T14:39:48Z

Thanks a lot ! Great addition to Pytorch Lightning. Would you mind adding it in pre-commit and resolving unit-tests ?

@tchaton when you mean resolving the unit-tests, do you mean applying the changes isort suggests? Because when I ran the pre-commit manually, a big part of the repo would be affected (160 files). Would we want to do it all in one PR, or run isort on the files as they are updated?

mergify · 2020-10-20T17:44:25Z

This pull request is now in conflict... :(

nathanpainchaud · 2020-10-20T17:47:24Z

I added the isort pre-commit hook to this PR, and applied the changes as well.

I didn't know of a way to give the authorship of the formatting commit to an automatic task rather than myself. If you would prefer it that way, I would be grateful if you could point me to indications on how to do so and I'll reapply the formatting.

codecov · 2020-10-20T18:30:25Z

Codecov Report

Merging #4242 (f5f9d1c) into master (baa8558) will increase coverage by 0%.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #4242   +/-   ##
======================================
  Coverage      93%     93%           
======================================
  Files         117     117           
  Lines        8914    8914           
======================================
+ Hits         8280    8283    +3     
+ Misses        634     631    -3

mergify · 2020-10-21T08:53:07Z

This pull request is now in conflict... :(

mergify · 2020-10-21T18:36:14Z

This pull request is now in conflict... :(

akihironitta · 2020-10-25T19:45:28Z

.github/workflows/code-formatting.yml

+      - name: Install isort
+        run: pip install isort==5.6.4
+      - name: Run isort
+        run: isort --settings-path=./pyproject.toml --diff .


Could you keep --check option to obtain UNIX exit status? Without --check, it always exits with 0, and it means that isort check always passes on GitHub Actions no matter what the source code is. I have checked the behaviour at https://github.com/akihironitta/ci-example/runs/1305737153.

Good catch. I assumed that --diff also caused returned an error code, since it doesn't modify the files in-place (much like --check-only in this sense). I didn't think to test it out. I've added back the --check-only flag.

Thank you for the swift action!

isort --help now has the description of the exit status of --check on the main branch! PyCQA/isort#1590

Checks the file for unsorted / unformatted imports and prints them to the command line without modifying the file. Returns 0 when nothing would change and returns 1 when the file would be reformatted.

mergify · 2020-10-26T18:12:16Z

This pull request is now in conflict... :(

nathanpainchaud · 2020-11-09T18:54:01Z

I saw that issue #4241 for this was closed, but that a PR with the same goal was merged in bolts. @Borda, @ananthsub, @tchaton, @akihironitta any updates on what you want me to do with this PR to get this merged?

nathanpainchaud · 2020-11-10T16:59:39Z

With the new configuration, isort only applies on root-level (i.e. setup.py), docs and benchmarks right now. That's also the only things the CI will check. However, the pre-commit hook is active, and every new file committed (across the whole repo, not just within the subdirectories mentioned) will be checked locally for sorted imports.

Eventually, we'll have to delete some glob expressions from the skipped files (https://github.com/nathanpainchaud/pytorch-lightning/blob/ci/4241_import_sort/pyproject.toml#L25-L29) to turn on the CI checks for them.

Borda · 2020-11-10T20:05:28Z

Eventually, we'll have to delete some glob expressions from the skipped files (https://github.com/nathanpainchaud/pytorch-lightning/blob/ci/4241_import_sort/pyproject.toml#L25-L29) to turn on the CI checks for them.

that could be a solution for us, step-by-step reduce the skip pattern, similar as we have type check
https://github.com/PyTorchLightning/pytorch-lightning/blob/master/.pyrightconfig.json

nathanpainchaud · 2020-11-10T20:16:46Z

step-by-step reduce the skip pattern

I'm glad that seems to be doing the trick for you! Given how isort functions, I don't even know (as of now) of any other way we could do it incrementally 😅

However, seeing as the pre-commit is still active for all files, I don't think it will be too long before most of the files' imports are sorted 😉

Borda · 2020-11-10T20:32:54Z

step-by-step reduce the skip pattern

I'm glad that seems to be doing the trick for you! Given how isort functions, I don't even know (as of now) of any other way we could do it incrementally 😅

I have not checked it yet, just a proposal to try

However, seeing as the pre-commit is still active for all files, I don't think it will be too long before most of the files' imports are sorted 😉

yes, but not everyone is using pre-commit... for this peace by peace changes you temporarily disable it and apply isort just for some files, right?

so let's change this PR such that it will introduce isort in pre-commit and in CI with ignoring all codebase, the create several smaller PRs wich would fix a few files and remove some patter from CI
cc: @PyTorchLightning/core-contributors

nathanpainchaud · 2020-11-10T21:11:17Z

for this peace by peace changes you temporarily disable it and apply isort just for some files, right?

Yes. Right now, I just applied isort to root-level (i.e. setup.py), docs and benchmarks to give a preview of the format used.

so let's change this PR such that it will introduce isort in pre-commit and in CI with ignoring all codebase, the create several smaller PRs wich would fix a few files and remove some patter from CI

What you're proposing is what the PR is doing in its current state (since the changes I made earlier today). The only files currently affected are the exceptions mentioned above. Tests, examples and the library itself are currently excluded from CI checks.

Borda

I like this initial step :]

.github/workflows/code-formatting.yml

rohitgr7

LGTM. Just a comment.

.github/workflows/code-formatting.yml

* added isort CI job and updated isort config * changed CI check output from files to full diff * added isort pre-commit hook * Added missing first party and restricted files affected by isort * Applied isort to root-level, docs and benchmarks * Apply suggestions from code review Co-authored-by: Nathan Painchaud <nathanpainchaud@gmail.com> Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> Co-authored-by: chaton <thomas@grid.ai>

mergify bot requested a review from a team October 19, 2020 20:38

Borda added the feature Is an improvement or enhancement label Oct 19, 2020

Borda reviewed Oct 19, 2020

View reviewed changes

setup.cfg Show resolved Hide resolved

mergify bot requested a review from a team October 19, 2020 21:46

edenlightning added this to the 1.0.3 milestone Oct 19, 2020

ydcjeff reviewed Oct 20, 2020

View reviewed changes

mergify bot requested a review from a team October 20, 2020 07:07

akihironitta reviewed Oct 20, 2020

View reviewed changes

tchaton requested changes Oct 20, 2020

View reviewed changes

nathanpainchaud requested review from tchaton and removed request for a team October 20, 2020 18:40

mergify bot requested a review from a team October 20, 2020 18:41

akihironitta reviewed Oct 25, 2020

View reviewed changes

nathanpainchaud requested review from ananyahjha93, awaelchli, justusschock, nateraw, SeanNaren, teddykoker and williamFalcon as code owners October 26, 2020 02:30

ananthsub approved these changes Oct 26, 2020

View reviewed changes

akihironitta mentioned this pull request Oct 31, 2020

ci: Check import formatting with isort Lightning-Universe/lightning-bolts#324

Merged

14 tasks

Nathan Painchaud added 2 commits November 10, 2020 11:51

Added missing first party and restricted files affected by isort

54f3aa5

Applied isort to root-level, docs and benchmarks

91e2b81

Borda approved these changes Nov 10, 2020

View reviewed changes

Borda reviewed Nov 10, 2020

View reviewed changes

.github/workflows/code-formatting.yml Outdated Show resolved Hide resolved

Apply suggestions from code review

b07f8fc

edenlightning modified the milestones: 1.0.x, 1.0.7 Nov 10, 2020

Borda modified the milestones: 1.0.7, 1.0.x Nov 11, 2020

rohitgr7 approved these changes Nov 11, 2020

View reviewed changes

rohitgr7 reviewed Nov 11, 2020

View reviewed changes

.github/workflows/code-formatting.yml Show resolved Hide resolved

edenlightning modified the milestones: 1.0.x, 1.0.7 Nov 13, 2020

Merge branch 'master' into ci/4241_import_sort

f5f9d1c

tchaton approved these changes Nov 13, 2020

View reviewed changes

Borda modified the milestones: 1.0.7, 1.0.x Nov 13, 2020

Borda added the ready PRs ready to be merged label Nov 13, 2020

Borda merged commit 2d78d9b into Lightning-AI:master Nov 13, 2020

nathanpainchaud deleted the ci/4241_import_sort branch November 14, 2020 01:15

akihironitta mentioned this pull request Nov 17, 2020

Apply import formatting to files in the 2nd top level #4717

Merged

11 tasks

akihironitta mentioned this pull request Nov 22, 2020

Apply import formatter isort #4805

Closed

30 tasks

akihironitta mentioned this pull request Dec 3, 2020

[isort] Improve import formatting configuration #4960

Closed

CI: Added isort import check for the code on pull-request #4242

CI: Added isort import check for the code on pull-request #4242

Conversation

nathanpainchaud commented Oct 19, 2020

What does this PR do?

Before submitting

PR review

Did you have fun?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akihironitta Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanpainchaud commented Oct 20, 2020 • edited Loading

mergify bot commented Oct 20, 2020

nathanpainchaud commented Oct 20, 2020

codecov bot commented Oct 20, 2020 • edited Loading

Codecov Report

mergify bot commented Oct 21, 2020

mergify bot commented Oct 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Oct 26, 2020

nathanpainchaud commented Nov 9, 2020

nathanpainchaud commented Nov 10, 2020

Borda commented Nov 10, 2020

nathanpainchaud commented Nov 10, 2020

Borda commented Nov 10, 2020 • edited Loading

nathanpainchaud commented Nov 10, 2020

Borda left a comment

Choose a reason for hiding this comment

rohitgr7 left a comment

Choose a reason for hiding this comment

akihironitta Oct 20, 2020 •

edited

Loading

nathanpainchaud commented Oct 20, 2020 •

edited

Loading

codecov bot commented Oct 20, 2020 •

edited

Loading

Borda commented Nov 10, 2020 •

edited

Loading