Emmit warning when assign/comparing string with Status Enum. #3875
Conversation
|
I'm expecting the tests to fail with many more areas where status is still compared with or assingned a string. |
|
I'm inclined to not warn just yet, and give downstream libraries a chance to react to this change. Thoughts? |
|
I'm fine not warning but I think that warnings (wel phrased with the right class) are a good and important communication channel, this is also why I did a PendingDeprecationWarning and not a DeprecationWarning. I'll also mostly use CI failure to find all the place that need updates. I can also update the message to say "We are thinking about deprecating ... blah blah", and let the deprecation in master but not release them so that downstread have chance to pitch in on non-release versions. |
|
Yeah, let me rephrase. I think that we should not impose warnings on users yet, at least not until we fix the other Dask sub-packages and have a release cycle or two. If you're using a warning class that is invisile to users under default settings then that seems sensble to me. |
In that case PendingDeprecationWarnings fits that definition. It is not visible in Repl or scripts. I believe since 3.7 only DeprecationWarnings are shown in REPL by default. |
| @@ -36,19 +36,39 @@ class ProcessInterface: | |||
| It should implement the methods below, like ``start`` and ``close`` | |||
| """ | |||
|
|
|||
| @property | |||
There was a problem hiding this comment.
Maybe instead we should implement this on the Cluster superclass in distributed/deploy/cluster.py
There was a problem hiding this comment.
That would also affect most of the downstream projects automatically, which would be useful.
There was a problem hiding this comment.
We could, but then ssh.Worker and ssh.Scheduler would not inherit this and I believe they were the reason I did this originally.
I think I might end up pulling this into a decorator that can be applied to classes maybe.
There was a problem hiding this comment.
Oh, my apologies. I saw the spec.py filename and assumed that this was the SpecCluster class. I see now that it is the ProcessInterface class instead. Happy to retract the comment.
I'd prefer avoiding the iterator and being explicit for now if that's ok, even given the duplication.
distributed/core.py
Outdated
| # result is not type stable: | ||
| # when LHS is not Status then RHS must not be Status or it raises. | ||
| # when LHS is Status then RHS must be status or it raises in tests | ||
| if reply and ininstance(result, Status) and result != Status.dont_reply: |
There was a problem hiding this comment.
I'm not super happy with this either.
|
Seem like the last few failures are around Status not being serialisable for msgpack. I'm not too sure how to write a custom serializer for that, and if that could be a deal breaker. |
Turn this into an error in the test suite. We want to make sure of various things: 1) original behavior of comparing/assign with string still works but emit warnings. 1b) assigning strings convert to proper enum variant. 2) assign/comparison with invalid strings should fail 2) warnings are errors in test suite to make sure we don't re-introduce strings. This is the continuation of dask#3853 Maybe Cluster in cluster.py should also get status as a property
|
I found here how to do custom packing. |
|
Yeah ! All tests passing. There are other places where messagepack is used, but those seem like the only two when those were needed. I defined in serialize as this is where most of msgpack serialisation lies. If you are happy with this, let me know and I can rebase/squash and update the version number in PeindingDeprecationWarning + something in the changelog. |
|
Going to ping on this; if you are still +1 on the approach, and if there is anything you want me to change. |
|
Sure. Let's give it a try. We're a couple weeks away from a release so we can see who this affects for a while and we'll have plenty of time to revert if necessary. |
Turn this into an error in the test suite.
We want to make sure of various things:
but emit warnings.
1b) assigning strings convert to proper enum variant.
strings.
This is the continuation of #3853