Remove update_graph_hlg #7502
Remove update_graph_hlg #7502
Conversation
| async def test_update_graph_culls(s, a, b): | ||
| s.update_graph( | ||
| # FIXME: Migrate to new API | ||
| s._update_graph_plain_tasks( | ||
| tasks={ | ||
| "x": dumps_task((inc, 1)), |
There was a problem hiding this comment.
This is the only place where actual functionality of update_graph was tested so I kept the original API call around. The functionality this is testing is interesting enough to be preserved since update_graph performs culling as well on top of whatever we're implementing in HLGs, etc.
I chose to keep it for now since I don't know yet what to do with it
cc @rjzamora in case you were not aware. Doesn't change too much about HLGs but this is interesting nonetheless, I believe
| dsk = {("x-%d" % i): (inc, i) for i in range(20)} | ||
| s.update_graph( | ||
| tasks=valmap(dumps_task, dsk), | ||
| keys=list(dsk), | ||
| dependencies={k: set() for k in dsk}, | ||
| ) |
There was a problem hiding this comment.
Most update_graph invocations where very old tests that were written at a time where the client was not what it is today. I migrated to a more modern version of these tests but kept functionality as is.
| @@ -142,35 +140,20 @@ async def test_multi_progressbar_widget(c, s, a, b): | |||
|
|
|||
|
|
|||
| @mock_widget() | |||
| @gen_cluster() | |||
| async def test_multi_progressbar_widget_after_close(s, a, b): | |||
There was a problem hiding this comment.
This test is a bit odd and I couldn't make sense of the suffix _after_close. One thing that is unique about this test is that it tests the multiprogress bars explicitly with intermediate keys so I adjusted accordingly
There was a problem hiding this comment.
Keeps failing. After some thought I'm not even sure if this is possible. I'll remove the test
| c._send_to_scheduler( | ||
| { | ||
| "op": "update-graph", | ||
| "tasks": {}, | ||
| "keys": [stringify(self.key)], | ||
| "client": c.id, | ||
| } | ||
| ) |
There was a problem hiding this comment.
From what I can tell this is not serving any purpose. The future itself lets the scheduler know that it is interested with a client_desires_keys so this is redundant and the only non-test invocation of the old API
| @gen_cluster() | ||
| async def test_filtered_communication(s, a, b): | ||
| c = await connect(s.address) | ||
| f = await connect(s.address) | ||
| await c.write({"op": "register-client", "client": "c", "versions": {}}) | ||
| await f.write({"op": "register-client", "client": "f", "versions": {}}) | ||
| await c.read() | ||
| await f.read() | ||
|
|
||
| assert set(s.client_comms) == {"c", "f"} | ||
|
|
||
| await c.write( | ||
| { | ||
| "op": "update-graph", | ||
| "tasks": {"x": dumps_task((inc, 1)), "y": dumps_task((inc, "x"))}, | ||
| "dependencies": {"x": [], "y": ["x"]}, | ||
| "client": "c", | ||
| "keys": ["y"], | ||
| } | ||
| ) | ||
|
|
||
| await f.write( | ||
| { | ||
| "op": "update-graph", | ||
| "tasks": { | ||
| "x": dumps_task((inc, 1)), | ||
| "z": dumps_task((operator.add, "x", 10)), | ||
| }, | ||
| "dependencies": {"x": [], "z": ["x"]}, | ||
| "client": "f", | ||
| "keys": ["z"], | ||
| } | ||
| ) | ||
| (msg,) = await c.read() | ||
| assert msg["op"] == "key-in-memory" | ||
| assert msg["key"] == "y" | ||
| (msg,) = await f.read() | ||
| assert msg["op"] == "key-in-memory" | ||
| assert msg["key"] == "z" |
There was a problem hiding this comment.
This was introduced in #108 and is long outdated.
We have plenty of these tests in test_client.py, e.g. test_multiple_clients
There was a problem hiding this comment.
I don't think this is worth being tested on a lower level
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 24 files ± 0 24 suites ±0 10h 32m 54s ⏱️ + 30m 17s For more details on these failures, see this check. Results for commit 5682371. ± Comparison against base commit 0161991. ♻️ This comment has been updated with latest results. |
|
Tests look good. The only failure is |
This removes the two methods update_graph and update_graph_hlg handlers. Now there is only one handler. A subsequent PR will consolidate the two methods.
I took the liberty of adding type annotations which makes the change larger than absolutely necessary but I think it's worth it.