Add isort for sorting imports

[hwpang]

Description

Add isort in dev dependency and check that imports are properly sorted in CI

Example / Current workflow

n/a

Bugfix / Desired workflow

n/a

Questions

n/a

Relevant issues

n/a

Checklist

• linted with flake8?
• (if appropriate) unit tests added?

[JacksonBurns]

I think this will fix the circular import, please try it and resort and then push (push so we can all see if it fails).

[JacksonBurns]

argh more circular imports... let's chat about this offline. ugh

[davidegraff]

aside from the circular import issues you seem to have, I have just one small comment. Otherwise, lgtm! thanks!

[hwpang]

@JacksonBurns I've figured out a way to fix the circular import while using isort (there is only one place that we cannot import alphabetically, so I add a tag to tell isort to skip), please review!

[KnathanM]

While we are sorting everything, would we also make sense to remove these extra lines between imports? I think we would still keep a line between the chemprop imports and other imports.

[davidegraff]

Almost all style guides define three sections of imports, which should be defined in the following order at the top of every file with a blank line between them:

1. standard library
2. third party
3. local imports

I personally define a 4th section, "sublocal," e.g., CLI subpackage imports which I view as logically separate from core library (e.g., chemprop.*) imports.

[hwpang]

Yeah, isort uses these sections: FUTURE,STDLIB,THIRDPARTY,FIRSTPARTY,LOCALFOLDER, see here

[KnathanM]

Did isort add this extra line? Why does it do that?

[hwpang]

Yeah, because isort uses these sections: FUTURE,STDLIB,THIRDPARTY,FIRSTPARTY,LOCALFOLDER. os is a standard library, and torch is a third party library.

[KnathanM]

Originally the order of all matched the order of imports? These changes breaks that symmetry, are we okay with that or should the order in all also be updated?

[davidegraff]

This is better IMO. The imports are in sorted order, but these are separate from the definitions this module exports. We should want the exports to also be in sorted order.

[hwpang]

Thanks for the discussion. I see that isort sorts __all__ with sort_reexports = true in the config file. However, running isort with sort_reexports = true breaks our file for some reason. The output is copied below. If I remove all the imports in the same file, __all__ is actually sorted correctly. What do you think to be the best way to proceed? I'm happy to take suggestions.

from .agg import (
    Aggregation,
    AggregationRegistry,
    AttentiveAggregat__all__ = ['Activation', 'Aggregation', 'AggregationRegistry', 'AtomMessagePassing', 'AttentiveAggregation',
 'BCELoss', 'BCEMetric', 'BinaryAUPRCMetric', 'BinaryAUROCMetric', 'BinaryAccuracyMetric',
 'BinaryClassificationFFN', 'BinaryClassificationFFNBase', 'BinaryDirichletFFN',
 'BinaryDirichletLoss', 'BinaryF1Metric', 'BinaryMCCLoss', 'BinaryMCCMetric', 'BondMessagePassing',
 'BoundedMAEMetric', 'BoundedMSELoss', 'BoundedMSEMetric', 'BoundedMixin', 'BoundedRMSEMetric',
 'CrossEntropyLoss', 'CrossEntropyMetric', 'DirichletMixin', 'EvidentialFFN', 'EvidentialLoss',
 'LossFunction', 'LossFunctionRegistry', 'MAEMetric', 'MSELoss', 'MSEMetric', 'MVELoss', 'MccMixin',
 'MeanAggregation', 'MessagePassing', 'Metric', 'MetricRegistry', 'MulticlassClassificationFFN',
 'MulticlassDirichletFFN', 'MulticlassDirichletLoss', 'MulticlassMCCLoss', 'MulticlassMCCMetric',
 'MulticomponentMessagePassing', 'MveFFN', 'NormAggregation', 'Predictor', 'PredictorRegistry',
 'R2Metric', 'RMSEMetric', 'RegressionFFN', 'SIDLoss', 'SIDMetric', 'SpectralFFN', 'SumAggregation',
 'ThresholdedMixin', 'UnscaleTransform', 'WassersteinLoss', 'WassersteinMetric']
classDirichletFFN,
    MveFFN,
    Predictor,
    PredictorRegistry,
    RegressionFFN,
    SpectralFFN,
)
from .transforms import UnscaleTransform
from .utils import Activation

__all__ = [

[davidegraff]

Almost all style guides define three sections of imports, which should be defined in the following order at the top of every file with a blank line between them:

1. standard library
2. third party
3. local imports

I personally define a 4th section, "sublocal," e.g., CLI subpackage imports which I view as logically separate from core library (e.g., chemprop.*) imports.

[davidegraff]

This is better IMO. The imports are in sorted order, but these are separate from the definitions this module exports. We should want the exports to also be in sorted order.