-
Notifications
You must be signed in to change notification settings - Fork 7.1k
[release][ci] First test for kuberay release test trigger path #54415
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
Conversation
Signed-off-by: kevin <[email protected]>
Signed-off-by: kevin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR extends the release test framework to support a KubeRay variant and updates unit tests accordingly.
- Test changes to initialize a KubeRay compute config and exercise the new
kuberayflag in error scenarios. - Glue code adds exception capture in the KubeRay path, records runtime, and maps exceptions to
Result.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| release/ray_release/tests/test_glue.py | Set up self.kuberay_test, extend _run to take a kuberay flag, and duplicate error-path tests for KubeRay. |
| release/ray_release/glue.py | Wrap KubeRay execution in try/except, record start_time/runtime, call handle_exception, and rethrow. |
Comments suppressed due to low confidence (3)
release/ray_release/tests/test_glue.py:320
- [nitpick] All error paths for KubeRay are covered, but there is no test for a successful KubeRay run. Consider adding a positive test to verify that
self._run(result, True)setsreturn_code == 0and populates expected fields.
self.assertEqual(result.return_code, ExitCode.CONFIG_ERROR.value)
release/ray_release/tests/test_glue.py:259
- [nitpick] The parameter name
kuberayis a bit ambiguous as a flag. Consider renaming to something likeuse_kuberayoris_kuberayfor clarity.
def _run(self, result: Result, kuberay: bool = False, **kwargs):
release/ray_release/tests/test_glue.py:317
- This assertion is indented inside the
with self.assertRaisesRegex(...)block, so it never runs after the exception is raised. Move it outside thewithat the same indentation level to ensure it's executed.
self.assertEqual(result.return_code, ExitCode.CONFIG_ERROR.value)
| working_dir_upload_path = upload_working_dir(get_working_dir(test)) | ||
| start_time = time.monotonic() | ||
| pipeline_exception = None | ||
| try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this wrapping does not really feel right.. to start, catching all Exception is not the right thing to do in most cases. could you explain what exactly you are trying to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using try/catch here mostly to retain the Exception that can be thrown any time commands run inside try. run_release_test_anyscale also implements the same thing. The purpose (I think) is to throw the right error code that matches with whatever errored out during the process (thus why it needs to retain the right exception msg)
Signed-off-by: kevin <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Kevin H. Luu <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Kevin H. Luu <[email protected]>
Signed-off-by: kevin <[email protected]>
…roject#54415) - Modify Kuberay release test trigger code to catch exception and store it in `Result` - Modify glue unit test to include Kuberay variant of the release test trigger path --------- Signed-off-by: kevin <[email protected]> Signed-off-by: Kevin H. Luu <[email protected]> Co-authored-by: Copilot <[email protected]> Signed-off-by: dshepelev15 <[email protected]>
…roject#54415) - Modify Kuberay release test trigger code to catch exception and store it in `Result` - Modify glue unit test to include Kuberay variant of the release test trigger path --------- Signed-off-by: kevin <[email protected]> Signed-off-by: Kevin H. Luu <[email protected]> Co-authored-by: Copilot <[email protected]> Signed-off-by: alimaazamat <[email protected]>
…roject#54415) - Modify Kuberay release test trigger code to catch exception and store it in `Result` - Modify glue unit test to include Kuberay variant of the release test trigger path --------- Signed-off-by: kevin <[email protected]> Signed-off-by: Kevin H. Luu <[email protected]> Co-authored-by: Copilot <[email protected]> Signed-off-by: Krishna Kalyan <[email protected]>
…roject#54415) - Modify Kuberay release test trigger code to catch exception and store it in `Result` - Modify glue unit test to include Kuberay variant of the release test trigger path --------- Signed-off-by: kevin <[email protected]> Signed-off-by: Kevin H. Luu <[email protected]> Co-authored-by: Copilot <[email protected]> Signed-off-by: jugalshah291 <[email protected]>
…roject#54415) - Modify Kuberay release test trigger code to catch exception and store it in `Result` - Modify glue unit test to include Kuberay variant of the release test trigger path --------- Signed-off-by: kevin <[email protected]> Signed-off-by: Kevin H. Luu <[email protected]> Co-authored-by: Copilot <[email protected]> Signed-off-by: Douglas Strodtman <[email protected]>
Result