Enable mypy in CI 2/2 #5348

crusaderky · 2021-09-24T15:59:34Z

Closes #2803.
Iterative follow-up to #5328.

scheduler.py should be reviewed thoroughly for performance regressions in the cythonised code.

jakirkham · 2021-09-24T16:10:17Z

distributed/scheduler.py

+        services: "dict | None" = None,
+        versions: "dict | None" = None,
+        extra: "dict | None" = None,


What does the Cython code around the usage of these variables look like? In the past I've seen Cython handle these "" types as just objects. If that's still true, that would mean Cython does not use the faster C level APIs associated with these objects, which would lose some of the performance gains we got with Cythonizing the scheduler.

You're right, they degrade performance. Reverted.

crusaderky · 2021-09-28T16:11:07Z

Moving the conversation from #5328

crusaderky · 2021-09-28T16:11:35Z

@jakirkham

So would recommend we leave scheduler.py alone.

Would like to, but can't. Adding # type: ignore at the top of the file breaks everything else. I could not make the --exclude parameter of mypy work either. So the only alternative to changing the actual annotations in scheduler.py is to add a wealth of # type: ignore on its individual lines.

As noted above, this is going to cause issues with Cythonization in the scheduler. If we can't leave that file alone, then unfortunately I don't think we are prepared to adopt MyPy yet.

Could you articulate how # type: ignore harms cythonization? For example, what harm does changing this

@cclass
class TaskGroup:
    _last_worker: WorkerState

    def __init__(self, name: str):
        self._last_worker = None

to this?

@cclass
class TaskGroup:
    _last_worker: WorkerState

    def __init__(self, name: str):
        self._last_worker = None  # type: ignore

note that I'm not advocating for changing _last_worker: WorkerState to _last_worker: "WorkerState | None", which as you pointed out does introduce degradation.

crusaderky · 2021-09-28T16:21:38Z

Notes from today's maintainers meeting:

At the moment, there's the problem that developers can easily introduce performance degradation in the Cythonized code every time they touch scheduler.py. Comparing side by side the HTML reports before/after the change is feasible only when one has a specific doubt, not as a general approach going forward.
A few solutions were proposed:

Have every PR that substantially alters scheduler.py be reviewed by @jakirkham (this is obviously rhetorical and not sustainable)
Ignore Cython performance to reduce the attrition on pure-Python development. A notable exception to this would be PRs like this one that primarily focus on type annotations, that should be as conservative as possible. Leave @jakirkham to patch it up in his own time at a later date.
Thoroughly cover individual scheduler methods with ASV. Whole-cluster performance measures can be trickier to write because the benefits of Cython will only become visible when the scheduler is the bottleneck, which is challenging to reproduce particularly on single-host CI environments.
Investigate and appropriate the performance reports that were produced for the Rust scheduler rewrite. Again running those measures on CI environments and getting meaningful insights out of them is likely going to be challenging.

crusaderky · 2021-09-29T11:45:04Z

Work in scheduler.py is done. All unit test failures are unrelated. Waiting for merge of #5328.

crusaderky · 2021-09-30T20:45:09Z

distributed/scheduler.py

    _transition_counter: Py_ssize_t
+    _plugins: dict  # dict[str, SchedulerPlugin]


Pulled up from Scheduler since SchedulerState was invoking self.plugins on many occasions

crusaderky · 2021-09-30T20:46:02Z

distributed/scheduler.py

+        unrunnable: set,
+        validate: bint,
+        plugins: "Iterable[SchedulerPlugin]" = (),
+        **kwargs,  # Passed verbatim to Server.__init__()


Removed a wealth of needless default values for init params

crusaderky · 2021-10-01T09:55:44Z

All test failures are unrelated.
To my understanding none of the changes should cause a slowdown to critical sections of the code.
Ready for final review.

ian-r-rose

Is there an upstream cython issue for why using stringy type annotations makes performance worse? Is it just because they haven't implemented it, or is there a deeper reason?

ian-r-rose · 2021-10-04T16:31:10Z

distributed/scheduler.py

@@ -5461,7 +5480,7 @@ def add_plugin(self, plugin=None, idempotent=False, name=None, **kwargs):

        """
        if isinstance(plugin, type):
-            plugin = plugin(self, **kwargs)
+            plugin = plugin(self, **kwargs)  # type: ignore


This could probably be fixed by capturing scheduler plugin kwargs in a ParamSpec and making a TypeVar for the add_plugin signature. I don't think we should actually do that here, though, since ParamSpec is in typing_extensions~=3.10, which is not universally supported yet.

mypy is right complaining here. This is genuinely wrong; most plugins DO NOT accept a scheduler positional arg.
See #5394 for proper fix

crusaderky · 2021-10-06T10:34:34Z

@jakirkham are you happy to merge this?

crusaderky · 2021-10-11T12:26:58Z

Merging on Wednesday if there are no further comments

crusaderky · 2021-10-11T13:41:49Z

distributed/worker.py

@@ -2517,6 +2517,8 @@ async def gather_dep(

                if not to_gather_keys:
                    return
+                assert cause


reverts #5160

crusaderky · 2021-10-11T13:42:04Z

docs/source/develop.rst

+
+from the root of the distributed repository. Now the code linters will be run each time
+you commit changes. You can skip these checks with ``git commit --no-verify`` or with
+the short version ``git commit -n``.


Align to dask/dask

crusaderky · 2021-10-13T09:16:34Z

In it goes!

jakirkham reviewed Sep 24, 2021

View reviewed changes

crusaderky mentioned this pull request Sep 27, 2021

Enable mypy in CI 1/2 #5328

Merged

crusaderky force-pushed the mypy2 branch from 8f737b4 to 70ff9b0 Compare September 30, 2021 20:09

crusaderky commented Sep 30, 2021

View reviewed changes

crusaderky force-pushed the mypy2 branch from f5a2908 to eedf6d8 Compare September 30, 2021 21:14

Enable mypy in CI 2/2

22e1fc8

crusaderky force-pushed the mypy2 branch from eedf6d8 to 22e1fc8 Compare September 30, 2021 22:17

crusaderky marked this pull request as ready for review October 1, 2021 09:55

fjetter approved these changes Oct 1, 2021

View reviewed changes

ian-r-rose reviewed Oct 4, 2021

View reviewed changes

Merge branch 'main' into mypy2

02156c9

crusaderky added 6 commits October 7, 2021 14:32

Merge branch 'main' into mypy2

37620d3

lint

33b45fc

Merge branch 'main' into mypy2

6b08def

Merge branch 'main' into mypy2

09dbeac

Run pyupgrade before black

c6e98af

Align docs to dask

da0c1f7

crusaderky commented Oct 11, 2021

View reviewed changes

crusaderky merged commit efbf888 into dask:main Oct 13, 2021

crusaderky deleted the mypy2 branch October 13, 2021 09:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable mypy in CI 2/2 #5348

Enable mypy in CI 2/2 #5348

crusaderky commented Sep 24, 2021 •

edited

Loading

jakirkham Sep 24, 2021

crusaderky Sep 28, 2021

crusaderky commented Sep 28, 2021

crusaderky commented Sep 28, 2021

crusaderky commented Sep 28, 2021

crusaderky commented Sep 29, 2021

crusaderky Sep 30, 2021

crusaderky Sep 30, 2021

crusaderky commented Oct 1, 2021

ian-r-rose left a comment

ian-r-rose Oct 4, 2021

crusaderky Oct 6, 2021

crusaderky commented Oct 6, 2021

crusaderky commented Oct 11, 2021

crusaderky Oct 11, 2021

crusaderky Oct 11, 2021

crusaderky commented Oct 13, 2021

		_transition_counter: Py_ssize_t
		_plugins: dict # dict[str, SchedulerPlugin]

Enable mypy in CI 2/2 #5348

Enable mypy in CI 2/2 #5348

Conversation

crusaderky commented Sep 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crusaderky commented Sep 28, 2021

crusaderky commented Sep 28, 2021

crusaderky commented Sep 28, 2021

crusaderky commented Sep 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crusaderky commented Oct 1, 2021

ian-r-rose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crusaderky commented Oct 6, 2021

crusaderky commented Oct 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crusaderky commented Oct 13, 2021

crusaderky commented Sep 24, 2021 •

edited

Loading