Add Cluster.called_from_running_loop and fix Cluster.asynchronous#7941
Conversation
| getattr(self.loop, "asyncio_loop", None) is asyncio.get_running_loop() | ||
| ) | ||
| except RuntimeError: | ||
| return self.asynchronous |
There was a problem hiding this comment.
this should be the value passed in def __init__(self, *, ..., asynchronous=False) not the computed attribute
There was a problem hiding this comment.
Ok I've updated it to store that in a private attribute to avoid folks using it instead of the property.
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 20 files ±0 20 suites ±0 13h 12m 42s ⏱️ + 53m 17s For more details on these failures, see this check. Results for commit e9601a9. ± Comparison against base commit 429ef8c. ♻️ This comment has been updated with latest results. |
| scheduler_sync_interval=1, | ||
| ): | ||
| self._loop_runner = LoopRunner(loop=loop, asynchronous=asynchronous) | ||
| self.__asynchronous = asynchronous |
There was a problem hiding this comment.
I'm slightly confused. The SyncMethodMixin is checking for _asynchronous but you chose to use an attribute with name mangling (double underscore) so this attribute will never be detected by the mixin. is this intentional?
There was a problem hiding this comment.
As far as I can see _asynchronous is never set anywhere apart from Client and Actor. So the mixin always falls back to the default for cluster objects. I went with the double underscore because I only wanted to use it for this one purpose and didn't want to collide with whatever _asynchronous is being used for, given that it was unclear to me what _asynchronous is even doing.
Happy to do something else here, do you have a suggestion?
There was a problem hiding this comment.
All good. Just wanted to make sure this is intentional. For this usecase the mangling is a good choice.
|
Thanks @jacobtomlinson for the fix and @fjetter @graingert for reviewing. I'll make sure this is included in the release later today |
jrbourbeau
changed the title
Cluster.called_from_running_loop and fix Cluster.asynchronous
4 participants
Closes #7940
This PR contains two changes:
SyncMethodMixin.asynchronousto returnFalseif not asynchronous and no loop is running.called_from_running_loopcheck out ofSpecCluster.__init__and into a property onClusterto allow for better reuse in subclasses.xref dask/dask-kubernetes#738
pre-commit run --all-files