Skip to content

Conversation

@sjperkins
Copy link
Member

sjperkins added 30 commits April 1, 2022 12:19
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

}
)

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?

@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