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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 7 commits into from
Nov 13, 2020
+49 −12
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4d271f9
added isort CI job and updated isort config
Oct 19, 2020
7ec72d3
changed CI check output from files to full diff
Oct 26, 2020
6852819
added isort pre-commit hook
Oct 19, 2020
54f3aa5
Added missing first party and restricted files affected by isort
Nov 10, 2020
91e2b81
Applied isort to root-level, docs and benchmarks
Nov 10, 2020
b07f8fc
Apply suggestions from code review
Borda Nov 10, 2020
f5f9d1c
Merge branch 'master' into ci/4241_import_sort
tchaton Nov 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .github/workflows/code-formatting.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,21 @@ on: # Trigger the workflow on push or pull request, but only for the master bra
branches: [master, "release/*"]

jobs:
imports-check-isort:
name: Check valid import formatting with isort
runs-on: ubuntu-20.04
steps:
- name: Checkout
uses: actions/checkout@v2
- name: Set up Python 3.8
uses: actions/setup-python@v2
with:
python-version: 3.8
- name: Install isort
run: pip install isort==5.6.4
rohitgr7 marked this conversation as resolved.
Show resolved Hide resolved
- name: Run isort
run: isort --settings-path=./pyproject.toml --check-only --diff .

code-black:
name: Check code formatting with Black
runs-on: ubuntu-20.04
Expand Down
9 changes: 9 additions & 0 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,12 @@ repos:
hooks:
- id: trailing-whitespace
- id: end-of-file-fixer

- repo: local
hooks:
- id: isort
name: isort
entry: python -m isort
args: [--settings-path, ./pyproject.toml]
language: system
types: [python]
2 changes: 1 addition & 1 deletion benchmarks/test_parity.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import tests.base.develop_utils as tutils
from pytorch_lightning import Trainer, seed_everything
from tests.base.models import ParityModuleRNN, ParityModuleMNIST
from tests.base.models import ParityModuleMNIST, ParityModuleRNN


@pytest.mark.parametrize('cls_model,max_diff', [
Expand Down
10 changes: 5 additions & 5 deletions docs/source/conf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,14 @@
# add these directories to sys.path here. If the directory is relative to the
# documentation root, use os.path.abspath to make it absolute, like shown here.

import os
import sys
# import m2r
import builtins
import glob
import shutil
import inspect
import os
import shutil
import sys

# import m2r
import builtins
import pt_lightning_sphinx_theme

PATH_HERE = os.path.abspath(os.path.dirname(__file__))
Expand Down
16 changes: 16 additions & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,19 @@ ignore = ["W504", "W504", "E402", "E731", "C40", "E741", "F40", "F841"]
line-length = 120
target-version = ["py38"]
exclude = "(.eggs|.git|.hg|.mypy_cache|.nox|.tox|.venv|.svn|_build|buck-out|build|dist)"

[tool.isort]
known_first_party = [
"bencharmks",
"docs",
"pl_examples",
"pytorch_lightning",
"tests",
]
skip_glob = [
"pl_examples/*",
"pytorch_lightning/*",
"tests/*"
] # Only apply formatting on root-level scripts, docs and benchmarks
profile = "black"
Copy link
Contributor

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

not back for codebase just for formating imports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

+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 :)

line_length = 120
1 change: 1 addition & 0 deletions requirements/test.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ check-manifest
twine==1.13.0
scikit-image>=0.17.2
black>=20.8b1
isort>=5.6.4
pre-commit>=1.0

cloudpickle>=1.2
Expand Down
5 changes: 0 additions & 5 deletions setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,3 @@ convention = pep257
# D202: Ignore a blank line after docstring (collision with Python Black in decorators)
add-ignore = D104,D107,D202
max-line-length = 120

[tool:isort]
Borda marked this conversation as resolved.
Show resolved Hide resolved
known_first_party = pytorch_lightning,tests
default_section=THIRDPARTY
line_length=120
3 changes: 2 additions & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@

import os
from io import open

# Always prefer setuptools over distutils
from setuptools import setup, find_packages
from setuptools import find_packages, setup

try:
import builtins
Expand Down