Consistent PEP8-compliant imports layout

[sisp]

This PR adds a pre-commit hook and CI jobs/steps to fix and check the Python imports layout so that it is consistent and PEP8-compliant. Different from my initial suggestion in #896 to use isort, I now use reorder-python-imports for reasons I'll discuss below. At this stage I consider this PR a basis for discussion about how to proceed.

isort is great for auto-formatting imports, but it only works well when it is installed run in the same virtual environment in which the dependencies of a project are installed.

isort when run in isolation is not the best at determining what dependencies are third party.

-- https://github.com/asottile/seed-isort-config#seed-isort-config

While runtime dependencies are centrally managed in setup.py and conda.recipe/meta.yaml, development dependencies are installed where needed on the fly and in different ways. For instance, pre-commit pulls black via its own mechanism while Travis installs black via pip while (before this PR) GitHub Actions use a community action for black. In all of these cases, Ignite and its dependencies are not installed along with its development dependencies, so isort wouldn't be able to properly determine third party dependencies.

One possible solution is to let seed-isort-config determine third party dependencies for isort before isort is run, but (at least to me) this feels like a small hack. An alternative solution - and the one I ended up using in this PR - is to use reorder-python-imports which appears to be doing a better job at determining third party dependencies.

Before this PR, Ignite mostly used several imports from the same package in one line, although there were cases where only one import per line was used. reorder-python-imports aims at reducing merge conflicts which is why it produces only one import per line. Is this a format you agree with?

At least for now reorder-python-imports only applies to Python files in ignite/** and tests/**. Formatting imports in examples/** leads to incorrect results because, e.g., import utils in examples/fast_neural_style/neural_style.py is classified as a third party dependency although it is a local module. There are many other cases like this. I suspect the misclassification is due to the non-standard project layout in examples/**.

There is a small downside to using reorder-python-imports though, in my opinion. At least VS Code's official Python extension does not seem to support it, so "format on save" won't work. And choosing isort in VS Code will lead to a different imports layout unless configured explicitly to match the layout produced by reorder-python-imports.

[vfdev-5]

@sisp thanks for the PR. I have no strong opinion about using isort or reorder-python-imports.
Have you an idea why isort is 10x rated than reorder-python-imports, but both projects started around 2013, 2014 ? Do you have some other projects that use reorder-python-imports?

Seems like we have a problem in accumulation.py that should import Metric as from ignite.metrics.metric import Metric.

[sisp]

Have you an idea why isort is 10x rated than reorder-python-imports, but both projects started around 2013, 2014 ?

No, I'm not sure why there is this large difference in rating. isort is much more configurable while reorder-python-imports is more opinionated, maybe similar to yapf vs. black.

Do you have some other projects that use reorder-python-imports?

Off the top of my head:

• python-oas
• yelp-python
• Werkzeug

But the author looks very solid to me.

Seems like we have a problem in accumulation.py that should import Metric as from ignite.metrics.metric import Metric.

Yes, there is an issue with circular imports which didn't show up before as imports were sorted in a way that didn't reveal the problem. A fix is coming in a second.

[vfdev-5]

@sisp so to better understand the problem with isort. So, to use it in the correct, we should use it as a step in the testing job (pytest tests/ ...) ? And we can not do the stuff as autopep8 without installing everything, so, user should apply it on his/her side before committing ?

Anyway, we can go with reorder-python-imports even if it is not so popular and VSCode does not support it. The essential thing is if it does its job correctly :)

[sisp]

The way I understand it, in order for isort to work correctly it needs to be installed along with the regular runtime dependencies (i.e. the third party dependencies). Since especially the pre-commit hooks and CI pipelines, that perform code quality checks, only install code quality tools without installing Ignite and its dependencies, isort doesn't work correctly. In order for isort to work correctly, pip install . && pip install isort would need to be run before running isort. Using isort as a pre-commit hook would still be problematic (as far as I know) because pre-commit installs its hooks in separate virtual environments, so isort would not recognize the packages installed in the developer's virtual environment. One solution is to let pre-commit use the version of isort installed in the developer's virtual environment like this:

- id: isort
  name: isort
  entry: isort -rc .
  language: system
  require_serial: true
  types: [python]

But for some reason I have yet to understand pre-commit is rarely used like this.

While this is getting slightly beyond the scope of this PR, in my opinion pre-commit should not install (most of) the hooks itself anyway. Instead, developers should install runtime and development dependencies in a virtual environment and tools like pre-commit simply use the installed tools. Similarly, CI pipelines should first install runtime and development dependencies in a virtual environment and then run the installed tools. This way, setup of development dependencies is managed in a single place along with version constraints (ideally pinned versions) to ensure reproducible development environments among developers. I use and like Poetry for managing runtime and development dependencies. The resolved dependency tree is automatically locked and committed to Git, so that developers can instantiate reproducible development environments. But this discussion really exceeds the scope of the PR. ;-)

[vfdev-5]

LGTM! Thanks @sisp !

[vfdev-5]

@sisp sorry for reiterating on that, after private discussions with the team, we prefer to keep multiline imports instead of single line (even taking into account the argument on git conflicts). Could you please configure the tool for that ? Thank you

changes needed

[sisp]

reorder-python-imports doesn't seem to support multiple imports per line, so I switched to isort and use isort-seed-config in .pre-commit-config.yaml to populate the known_third_party field in setup.cfg. What do you think?

[vfdev-5]

@sisp thanks a lot for working on that ! Yes, seems like reorder-python-imports does not support multiline imports.
Can we opt to set https://github.com/timothycrosley/isort#intelligently-balanced-multi-line-imports instead of having

from ignite.contrib.handlers import (
    MLflowLogger,
    PolyaxonLogger,
    ProgressBar,
    TensorboardLogger,
    VisdomLogger,
    global_step_from_engine,
)

[sisp]

Just to clarify: Would you like a different multi-line output mode (e.g. "0 - Grid") or is all you'd like the setting balanced_wrapping=True as stated in "intelligently balanced multi-line imports"? The latter does not cause any change when re-running isort at the moment.

[vfdev-5]

Well, idea is to keep the imports as "0 - Grid" and "Intelligently Balanced Multi-line Imports" seems to be the best fit...

[sisp]

I just noticed this setting multi_line_output=3 in setup.cfg which leads to vertical hanging indent. I copied this setting from the black documentation which has a note on compatibility of black and isort. When I change the setting to multi_line_output=0, black and isort seem to want different things, i.e. isort formats the imports as "0 - Grid" and then black formats them back to "3 - Vertical Hanging Indent".

[vfdev-5]

@sisp can we do something with that in order to keep "0 - Grid" ?

[sisp]

I don't think so because black cannot be configured in this way. In fact, you've never had the "0 - Grid" layout because of black which I didn't introduce (but definitely recommend keeping!).

[vfdev-5]

I don't think so because black cannot be configured in this way. In fact, you've never had the "0 - Grid" layout because of black which I didn't introduce (but definitely recommend keeping!).

OK, I see what you mean. Seems like two tools can not play well without multiplying the number of imports...
The only solution I see is to modify the code and limit the number of import from the same module, however does it mean a better code to satisfy tool's limitations...

[sisp]

Could you elaborate a bit on what you dislike about the way isort formats imports? For instance, do you not like this change? https://github.com/pytorch/ignite/pull/901/files#diff-eea3d16ed9ec4b7e9fe5e84be4f6aa1d

Before this PR, the imports layout was not consistent, e.g.:

-from ignite.contrib.handlers import ProgressBar
-from ignite.contrib.handlers import VisdomLogger
-from ignite.contrib.handlers import TensorboardLogger, global_step_from_engine
-from ignite.contrib.handlers import MLflowLogger
-from ignite.contrib.handlers import PolyaxonLogger
+from ignite.contrib.handlers import (
+    MLflowLogger,
+    PolyaxonLogger,
+    ProgressBar,
+    TensorboardLogger,
+    VisdomLogger,
+    global_step_from_engine,
+)

I don't see a limitation in isort here. isort makes imports formatting consistent. I'd always go with automated code formatting tools (as long as they're doing a reasonably good job, which I think black and isort are) as you during code review and contributors during implementation don't need to worry about code formatting.

If you feel there are too many imports in a module, perhaps the module needs to be refactored or modules rather than specific classes/functions should be imported. In fact, there are inconsistencies of that sort right now, e.g.:

from ignite.contrib.handlers import MLflowLogger
import ignite.contrib.handlers.mlflow_logger as mlflow_logger_module

[vfdev-5]

@sisp well, IMO, my perfect reformatting would do the following:

-from ignite.contrib.handlers import ProgressBar
-from ignite.contrib.handlers import VisdomLogger
-from ignite.contrib.handlers import TensorboardLogger, global_step_from_engine
-from ignite.contrib.handlers import MLflowLogger
-from ignite.contrib.handlers import PolyaxonLogger
+from ignite.contrib.handlers import MLflowLogger, PolyaxonLogger, ProgressBar
+from ignite.contrib.handlers import TensorboardLogger, VisdomLogger, global_step_from_engine

Yes, I agree that in some cases we can rework the code. But what can we do in this case for example in tests, when we need to import all classes:

from ignite.contrib.handlers.param_scheduler import (
    ConcatScheduler,
    CosineAnnealingScheduler,
    LinearCyclicalScheduler,
    LRScheduler,
    ParamGroupScheduler,
    PiecewiseLinear,
    create_lr_scheduler_with_warmup,
)

Maybe from ignite.contrib.handlers.param_scheduler import *... (i'm not fan)

[sisp]

Interesting, I've never seen this layout. I actually prefer one import per line which means multiple lines for importing multiple classes/functions from the same module. It is consistent and also works for quite long import statements. But I guess it's a matter of taste.

[sisp]

While looking at the code at bit more, I find it odd to pass a module to a function:

ignite/ignite/contrib/engines/common.py

Line 209 in 32275a8

def setup_any_logging(logger, logger_module, trainer, optimizers, evaluators, log_every_iters):

Sure, this can be done in Python, but it means a logger module must implement a particular interface. Also, a class constructor like OutputHandler must implement an interface which I don't think is common (and not even possible in many languages). Perhaps a logger should contain factory methods like create_output_handler which must have the same signature for all loggers (could be enforced by making BaseLogger an ABC and methods like create_output_handler abstract methods). This would also hide the OutputHandler constructor and importing the entire module for a logger would not be necessary anymore.

[vfdev-5]

Also, a class constructor like OutputHandler must implement an interface which I don't think is common (and not even possible in many languages).

OutputHandler is an implementation of an abstract class :

ignite/ignite/contrib/handlers/tensorboard_logger.py

Line 27 in 32275a8

class OutputHandler(BaseOutputHandler):

I agree that setup_any_logging(logger, logger_module, ...) looks a bit odd.

Perhaps a logger should contain factory methods like create_output_handler which must have the same signature for all loggers (could be enforced by making BaseLogger an ABC and methods like create_output_handler and abstract method).

I see what you mean. Let me think about it...

[sisp]

True, I just don't think of constructors as part of a public interface, i.e. I can't enforce the signature of a constructor.

[sisp]

By the way, I'm not sure why tests are failing. GitHub just says "This check failed". Do you know what's wrong?

[vfdev-5]

By the way, I'm not sure why tests are failing. GitHub just says "This check failed". Do you know what's wrong?

Maybe github service is out... I do not know. I restarted them
https://www.githubstatus.com

[vfdev-5]

@sisp could you please update this PR as we updated ignite/ignite/contrib/engines/common.py ?

[vfdev-5]

@sisp thanks for the PR !
I used your initial commits and integrated recent changes discussed previously.