[release test] collapse into AnyscaleJobRunner in glue.py

[aslonnie]

it is always an instance of AnyscaleJobRunner.

[gemini-code-assist]

Code Review

This pull request refactors the code to use AnyscaleJobRunner directly instead of the more generic CommandRunner, which simplifies the logic by removing several type checks. The changes are generally good and improve code clarity.

However, I've found a critical issue where removing one of the checks without replacing it with a null-check can lead to an AttributeError if the runner object is None. This could happen during error handling and would shadow the original exception. I've provided a suggestion to fix this.

Additionally, I've pointed out a newly added isinstance check that appears to be redundant given the function's type hints and logic, suggesting an alternative if an assertion is desired.

[gemini-code-assist]

These assignments are not safe if runner is None. This can happen if an exception is raised before runner is assigned in the try block. Accessing runner.job_manager would then raise an AttributeError, shadowing the original, more informative exception. You should guard these assignments.

Suggested change

	result.job_url = runner.job_manager.job_url
	result.job_id = runner.job_manager.job_id
	result.last_logs = runner.get_last_logs() if runner else None
	result.job_url = runner.job_manager.job_url if runner else None
	result.job_id = runner.job_manager.job_id if runner else None
	result.last_logs = runner.get_last_logs() if runner else None

[gemini-code-assist]

This isinstance check seems redundant. The function's return type hint already specifies AnyscaleJobRunner, and the logic within this function ensures that command_runner is an instance of AnyscaleJobRunner. If the goal is to assert this, an assert isinstance(command_runner, AnyscaleJobRunner) would be more idiomatic for an internal check that should always pass. Raising ReleaseTestSetupError suggests this is a user-configurable error, which doesn't seem to be the case here.

[andrew-anyscale]

This could happen because we wrap the above init in a try/catch. https://github.com/ray-project/ray/pull/60267/files#diff-542132be3c6f0d88893d58435c5460eb5ec1a28999b98d30a87e8e8856e26965R551-R555

What should the outcome be if _load_test_configuration fails?

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

[cursor]

Diagnostic info lost when test execution fails

High Severity

Moving the code that sets result.job_url, result.job_id, and result.last_logs inside the try block means these diagnostic values won't be captured when an exception occurs during test execution. Previously, this code ran unconditionally after the try-except, ensuring job info and logs were available for debugging failed tests. Now, when tests fail (the most critical scenario for debugging), last_logs falls back to just the exception traceback instead of actual job logs, and job_url/job_id are not set at all.

 

[aslonnie]

This could happen because we wrap the above init in a try/catch. https://github.com/ray-project/ray/pull/60267/files#diff-542132be3c6f0d88893d58435c5460eb5ec1a28999b98d30a87e8e8856e26965R551-R555

What should the outcome be if _load_test_configuration fails?

moved into the try block.