[release test] remove legacy job runner dict in glue.py #58718
Conversation
| self._run(result, True) | ||
| self.assertEqual(result.return_code, ExitCode.CONFIG_ERROR.value) | ||
|
|
||
| def testStartClusterFails(self): |
There was a problem hiding this comment.
cluster lifecycle management is on anyscale now.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the legacy JobRunner and FullClusterManager, simplifying the codebase to only use AnyscaleJobRunner for release tests. The changes are clean and correctly remove the associated logic and tests. I've added a couple of suggestions for further simplification now that the legacy path is gone. Specifically, I've pointed out a redundant conditional check and an opportunity to remove single-entry dictionaries to make the code more direct.
| type_str_to_command_runner = { | ||
| "job": JobRunner, | ||
| "anyscale_job": AnyscaleJobRunner, | ||
| } | ||
|
|
||
| command_runner_to_cluster_manager = { | ||
| JobRunner: FullClusterManager, | ||
| AnyscaleJobRunner: MinimalClusterManager, | ||
| } |
There was a problem hiding this comment.
Now that JobRunner has been removed, these dictionaries only contain a single entry. This adds a layer of indirection that is no longer necessary. Consider removing these dictionaries and directly using AnyscaleJobRunner and MinimalClusterManager in _load_test_configuration. This would make the control flow more straightforward.
For example, you could refactor the logic in _load_test_configuration to something like this:
run_type = test["run"].get("type", DEFAULT_RUN_TYPE)
if run_type == "anyscale_job":
command_runner_cls = AnyscaleJobRunner
cluster_manager_cls = MinimalClusterManager
else:
raise ReleaseTestConfigError(
f"Unknown command runner type: {run_type}. Must be one of "
f"['anyscale_job']"
)| if isinstance(command_runner, AnyscaleJobRunner): | ||
| command_runner.job_manager.cluster_startup_timeout = cluster_timeout |
There was a problem hiding this comment.
Since AnyscaleJobRunner is now the only command runner used in this path, the isinstance check is redundant. You can simplify the code by removing the conditional statement.
command_runner.job_manager.cluster_startup_timeout = cluster_timeout
Signed-off-by: Lonnie Liu <[email protected]>
aslonnie
force-pushed
the
lonnie-251117-nooldjob
branch
from
2ed965d to
1f61281
Compare
ray-gardener
bot
added
core
Signed-off-by: Lonnie Liu <[email protected]>
…58718) not being used any more since a loong time ago. Signed-off-by: Lonnie Liu <[email protected]> Signed-off-by: Aydin Abiar <[email protected]>
…58718) not being used any more since a loong time ago. Signed-off-by: Lonnie Liu <[email protected]> Signed-off-by: YK <[email protected]>
…58718) not being used any more since a loong time ago. Signed-off-by: Lonnie Liu <[email protected]>
not being used any more since a loong time ago.