Ensure exceptions are raised if workers are incorrectly started

[fjetter]

Server inherits from object which has no sensible initialiser. In particular, when provided with kwargs, it raises an obscure TypeError

TypeError: object.__init__() takes exactly one argument (the instance to initialize)

Instead of raising, this would issue a warning indicating that some arguments were not used

[jrbourbeau]

Thanks @fjetter -- this would close #4640 (cc @gjoseph92)

[jrbourbeau]

Hmm I think the call to super().__init__ was intentional -- see https://github.com/dask/distributed/pull/4365/files/2212edc11b424c17f2f656eb73b2ca206742705a#r557630488 for an earlier discussion on this point

[fjetter]

multiple inheritance is a wonderful monstrosity :D

In [1]: class A:
   ...:     def __init__(self, a, **kwargs):
   ...:         print(f"A: {kwargs}")
   ...:

In [2]: class B(A):
   ...:     def __init__(self, b, **kwargs):
   ...:         print(f"B: {kwargs}")
   ...:         super().__init__(**kwargs)
   ...:

In [3]: class C(A):
   ...:     def __init__(self, c, **kwargs):
   ...:         print(f"C: {kwargs}")
   ...:         super().__init__(**kwargs)
   ...:

In [4]: class D(B, C):
   ...:     def __init__(self, d, **kwargs):
   ...:         print(f"D: {kwargs}")
   ...:         super().__init__(**kwargs)
   ...:

In [5]: D.mro()
Out[5]: [__main__.D, __main__.B, __main__.C, __main__.A, object]

In [6]: D(a="a", b="b", c="c", d="d")
D: {'a': 'a', 'b': 'b', 'c': 'c'}
B: {'a': 'a', 'c': 'c'}
C: {'a': 'a'}
A: {}

kwargs will trickle down the mro. therefore we need to allow kwargs everwhere. This is true for all classes but the root. the root cannot call super with kwargs since object doesn't allow arguments. afaik, the init on object is a no-op

[jrbourbeau]

Yep, that makes sense. IIUC the call to super was added here to make us more resilient to the inheritance order. For example, today Scheduler has the following class inheritance structure

distributed/distributed/scheduler.py

Line 3169 in 42e3f22

class Scheduler(SchedulerState, ServerNode):

So Server is at the bottom of the mro. Though I don't think we're strictly enforcing this anywhere. Perhaps we should though.

[gjoseph92]

We could consider adding a test like this (untested):

def test_server_inheritance():
    subclasses = Server.__subclasses__()
    for subcls in subclasses:
        assert subcls.__mro__[-2:] == [Server, object], f"Server must always be the last class in the MRO. {subcls} has MRO: {subcls.__mro__}"

Obviously this would depend on all Server subclasses being imported at the time the test runs.

Alternatively, if we wanted to over-engineer it a bit, we could use __init_subclass__ (added in 3.6) to enforce this. I've done something like this before here. Not sure how it would play with Cython though?

[gjoseph92]

Thanks for this @fjetter. I agree with you that removing super().__init__ is fine, since Server is always at the root of the MRO—so long as we don't significantly change the inheritance structure in the future.

[jrbourbeau]

If we're removing the super call then perhaps we should also remove kwargs in the signature and error loudly when an unexpected keyword is given

[fjetter]

If we're removing the super call then perhaps we should also remove kwargs in the signature and error loudly when an unexpected keyword is given

Sure. I was assuming we had this to allow for easier forward compatible code but I would prefer to be strict as well.

[fjetter]

I removed the kwargs entirely. I was about to delete the tests but discovered this interesting edge case we didn't properly cover. If the local cluster is provided with foo, it is passed to the worker. If one uses a nanny this is entirely swallowed and the nanny simply doesn't come up. I'm not even sure if an exception was logged...

[jrbourbeau]

cc @jakirkham for visibility

[fjetter]

One thing which came up in two comments is to add test about whether Server has a superclass other than object. I feel this is overengineered and I would expect this to pop up quite naturally if somebody constructs a more complicated inheritance.
IMHO if somebody wants to create more complicated inheritance model I believe this would not go unnoticed. That would be a major change in the software design and I guess more often than not the composition over inheritance principle should then rather guide us than introducing a deeper, branched off inheritance.

However, if people feel this is useful, I don't mind adding a test, that should be easy enough

[fjetter]

There are some related CI failures on OSX I'll need to check out :( (They are not Cython related)

[fjetter]

The failing test was an annoying one. There is a race condition between the process.on_exit / mark_stopped and the init_q exception forwarding/reraising. If the process is cleaned up too fast, the status of the nanny is set to closed and queue is cleaned up faster than it is able to read the message and raise the exception. That was also the cause for the other flaky test. I don't like my solution but it is a pragmatic one. Everything else I can come up with would either require more sync primitives or some bigger refactoring. If somebody smarter has a good idea, I'm all ears.

[fjetter]

This big diff is mostly wrapping the function in a try/except

[fjetter]

This is new

[fjetter]

This resolves the race condition in test_worker_start_exception

[fjetter]

This resolves the race condition in test_failure_during_worker_initialization

[fjetter]

This PR escalated pretty drastically, considering I simply wanted to cleanup the signature of a class 😅 I therefore changed the title to the more significant change

[fjetter]

If there are no further objections, I'd like to merge this.

Friendly ping @jakirkham

[jakirkham]

I haven't looked at the Nanny or Worker code, but the constructor change seems ok to me. Updated that thread and marked resolved

[jrbourbeau]

Thanks @fjetter! This is in