Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stimulus id contextvars explicit handlers #6083

Closed

Conversation

sjperkins
Copy link
Member

inspect.iscoroutinefunction doesn't recognise cythonised async functions
This reverts commit 1ce45f0.
@sjperkins
Copy link
Member Author

@fjetter Would you mind taking a brief look at the approach here and giving me your opinion on the overall complexity required here? I think looking at scheduler.py and test_scheduler.py will give you 95% of the flavour.

Essentially, explicit handlers requiring stimulus_id's are created that call implementations.

So for e.g. there's an explicit handle_update_graph function that calls update_graph. In practice this means that more stimulus_id's must be supplied in the test cases (and client.py). Also many of the tests are modified to call their handler instead of the implementation e.g. s.update_graph(...) changes to s.handle_update_graph(..., stimulus_id="test")

@sjperkins
Copy link
Member Author

This is based on #6046

distributed/utils.py Outdated Show resolved Hide resolved
distributed/utils.py Outdated Show resolved Hide resolved
@@ -8204,6 +8266,34 @@ def request_remove_replicas(self, addr: str, keys: list, *, stimulus_id: str):
}
)

handle_update_graph_hlg = expect_stimulus(sync=True)(update_graph_hlg)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we should not involve the Client at this point. There is almost no additional information avialable if the client generates an ID or if the scheduler just generates it.
We wouldn't need to define this many handlers if the client is not involved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are sending other stimuli from the client though? e.g. key release stimuli?

With handlers explicitly requiring stimuli to be sent in this PR, it's necessary to do so in other places in the Client too. Why not complete the loop on all stimuli?

distributed/scheduler.py Outdated Show resolved Hide resolved
distributed/scheduler.py Outdated Show resolved Hide resolved
distributed/scheduler.py Outdated Show resolved Hide resolved
distributed/scheduler.py Outdated Show resolved Hide resolved
distributed/scheduler.py Outdated Show resolved Hide resolved
@sjperkins sjperkins force-pushed the stimulus-id-contextvars-explicit-handlers branch from 9b08e29 to 1ddffb0 Compare April 7, 2022 15:24
finally:
STIMULUS_ID.reset(token)


Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be removed

await self.remove_worker(address=ws._address)
await self.handle_remove_worker(
address=ws._address, stimulus_id=f"check-worker-ttl-{time()}"
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pehaps check_worker_ttl should be decorated with expects_stimulus? I guess an initialising stimulus could be provided when setting up the PeriodicCallback

@github-actions
Copy link
Contributor

github-actions bot commented Apr 8, 2022

Unit Test Results

       18 files  ±  0         18 suites  ±0   9h 19m 39s ⏱️ - 1m 55s
  2 726 tests +  2    2 640 ✔️  -   2       82 💤 +1  4 +3 
24 382 runs  +18  23 160 ✔️ +11  1 218 💤 +4  4 +3 

For more details on these failures, see this check.

Results for commit bfcbdb0. ± Comparison against base commit 6e30766.

♻️ This comment has been updated with latest results.

@sjperkins
Copy link
Member Author

Difficult to get ContextVar interaction with Tornado right in add_callback methods. See for e.g. tornadoweb/tornado#2731. Closing in favour of #6095

@sjperkins sjperkins closed this Apr 8, 2022
@sjperkins sjperkins deleted the stimulus-id-contextvars-explicit-handlers branch April 8, 2022 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transition tracing for scheduler task transitions
2 participants