Add isort to pre-commit hooks, package resorting

[charlesbluca]

This PR adds isort to the pre-commit hooks and attempts to resolve any resulting conflicts this would create. For the most part, not a lot of manual changes had to be made to do this:

• in distributed/deploy/tests/test_local.py, a separate import of distributed.utils_test.loop was grouped with all other imports from distributed.utils_test
• in distributed/tests/test_client.py, some additional flake8 noqas were added to ignore imports marked as unused
• in distributed/__init__.py, the from . import config is pinned at the top to ensure configuration is done properly

Not sure if isort should also be added to the CI.

• Tests added / passed
• Passes black distributed / flake8 distributed

[charlesbluca]

Trying to wrap my head around the failures, which all seem to be based around the same failure:

   File "/home/runner/work/distributed/distributed/distributed/__init__.py", line 6, in <module>
    from .actor import Actor, ActorFuture
  File "/home/runner/work/distributed/distributed/distributed/actor.py", line 6, in <module>
    from .client import Future, default_client
  File "/home/runner/work/distributed/distributed/distributed/client.py", line 43, in <module>
    from .batched import BatchedSend
  File "/home/runner/work/distributed/distributed/distributed/batched.py", line 8, in <module>
    from .core import CommClosedError
  File "/home/runner/work/distributed/distributed/distributed/core.py", line 20, in <module>
    from . import profile, protocol
  File "/home/runner/work/distributed/distributed/distributed/profile.py", line 37, in <module>
    from .utils import color_of, format_time, parse_timedelta
  File "/home/runner/work/distributed/distributed/distributed/utils.py", line 102, in <module>
    mp_context = _initialize_mp_context()
  File "/home/runner/work/distributed/distributed/distributed/utils.py", line 82, in _initialize_mp_context
    method = dask.config.get("distributed.worker.multiprocessing-method")
  File "/usr/share/miniconda3/envs/dask-distributed/lib/python3.8/site-packages/dask/config.py", line 475, in get
    result = result[k]
KeyError: 'distributed'

Does this have to do with the ordering of the config imports in distributed/__init__.py?

distributed/distributed/__init__.py

Lines 1 to 4 in de60c90

	from . import config
	import dask
	from dask.config import config
	from .actor import Actor, ActorFuture

[jrbourbeau]

Thanks for the PR @charlesbluca!

Could I ask you coordinate with @jsignell over in dask/dask#7370 to make sure we use the same isort settings in both dask and distributed?

Regarding the config errors, I think you're right that the location of from . import config matters in distributed/__init__.py as that's where distributed adds it's configuration settings into Dask's config system.

[jrbourbeau]

Instead of adding isort options here I'd prefer if we included them in setup.cfg (similar to Xarray and pandas). This was when a user runs isort locally it will pick up the particular settings we want.

[charlesbluca]

Sure! I ended up using the same general configuration from dask#7370 here.

[charlesbluca]

Thanks for the review! Made alterations to keep isort version and settings consistent with dask#7370, and pinned the local config import to the top of distributed/__init__.py, which retains the original order of imports.

[charlesbluca]

One error (truncated) on windows 3.8 tests:

_______________________ test_as_completed_with_results ________________________

client = <Client: 'tcp://127.0.0.1:56402' processes=2 threads=2, memory=15.03 GB>

    def test_as_completed_with_results(client):
        x = client.submit(throws, 1)
        y = client.submit(inc, 5)
        z = client.submit(inc, 1)
    
        ac = as_completed([x, y, z], with_results=True)
        y.cancel()
        with pytest.raises(RuntimeError) as exc:
>           res = list(ac)
E           Failed: DID NOT RAISE <class 'RuntimeError'>

distributed\tests\test_as_completed.py:193: Failed

...

distributed.scheduler - INFO - Client Client-02fe9e87-87f4-11eb-96d8-000d3a14453a requests to cancel 1 keys

distributed.scheduler - INFO - Scheduler cancels key inc-aa0231dc9b3c3d81591e1fe036cfdf54.  Force=False

distributed.scheduler - ERROR - Couldn't gather keys {'inc-aa0231dc9b3c3d81591e1fe036cfdf54': []} state: [None] workers: []

NoneType: None

distributed.scheduler - ERROR - Workers don't have promised key: [], inc-aa0231dc9b3c3d81591e1fe036cfdf54

NoneType: None

distributed.client - WARNING - Couldn't gather 1 keys, rescheduling {'inc-aa0231dc9b3c3d81591e1fe036cfdf54': ()}
distributed.core - ERROR - 'NoneType' object has no attribute '_key'

Traceback (most recent call last):

  File "D:\a\distributed\distributed\distributed\core.py", line 572, in handle_stream

    handler(**merge(extra, msg))

  File "D:\a\distributed\distributed\distributed\scheduler.py", line 5859, in report_on_key

    report_msg: dict = _task_to_report_msg(parent, ts)

  File "D:\a\distributed\distributed\distributed\scheduler.py", line 6861, in _task_to_report_msg

    return {"op": "cancelled-key", "key": ts._key}

AttributeError: 'NoneType' object has no attribute '_key'

...

Is this a flaky test or could this be related to the import order somewhere in core.py or scheduler.py?

[hristog]

Hi @charlesbluca, I'm not a maintainer, but perhaps you could try to push an empty commit to see if that same test would fail again via, e.g., git commit --allow-empty -m "Trigger CI"?

test_as_completed_with_results indeed does exhibit some characteristics of a flaky test, but I haven't had a chance to go through all the changes yet to have a better idea about how the changes might've affected it, just in terms of one CI-matrix combination.

If it doesn't fail again, it could perhaps be labeled as flaky (as part of this PR or otherwise).

[charlesbluca]

That's a good idea @hristog - I actually might end up closing this PR and reopening after fetching from upstream as there are a lot of merge conflicts piling up - I will check if test_as_completed_with_results ends up failing again though.

[charlesbluca]

Opened up #4647 with a second attempt at this.