[Job] add driver exit code to job info #39675

jonathan-anyscale · 2023-09-14T22:55:28Z

Why are these changes needed?

Add driver exit code to JobInfo class which handled by JobManager

Related issue number

Closes #31183

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
- I've added any new APIs to the API Reference. For example, if I added a
  method in Tune, I've added it in doc/source/tune/api/ under the
  corresponding .rst file.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>

dashboard/modules/job/common.py

dashboard/modules/job/job_manager.py

Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>

jjyao

lg

dashboard/modules/job/job_manager.py

dashboard/modules/job/tests/test_job_manager.py

Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>

architkulkarni

Looks good overall, just minor comments/questions

architkulkarni · 2023-09-22T16:36:57Z

dashboard/modules/job/pydantic_models.py

@@ -98,3 +98,7 @@ class JobDetails(BaseModel):
        None,
        description="The node ID of the node the job entrypoint command is running on.",
    )
+    driver_exit_code: Optional[str] = Field(


Out of curiosity why Optional[str] and not Optional[int]? Also, in the description we should also define when it would return None.

good catch, the actual output is int so shld update that

architkulkarni · 2023-09-22T16:38:31Z

dashboard/modules/job/common.py

@@ -95,6 +95,8 @@ class JobInfo:
    #: The node id that driver running on. It will be None only when the job status
    # is PENDING, and this field will not be deleted or modified even if the driver dies
    driver_node_id: Optional[str] = None
+    #: The driver process exit code after driver terminates


Please define in the comment when this would be None. I think this is what shows up in the auto-generated Ray docs, but it would be good to confirm this and see if there's anywhere else in the docs that we need to update

architkulkarni · 2023-09-22T16:39:42Z

dashboard/modules/job/tests/test_job_manager.py

@@ -908,6 +922,8 @@ async def test_failed_job(self, job_manager):
            await async_wait_for_condition_async_predicate(
                check_job_failed, job_manager=job_manager, job_id=job_id
            )
+            data = await job_manager.get_job_info(job_id)
+            assert data.driver_exit_code == -9


Can you add a comment about the meaning of -9 and why it's expected?

architkulkarni · 2023-09-22T16:45:36Z

src/ray/protobuf/gcs.proto

@@ -380,6 +380,8 @@ message JobsAPIInfo {
  // The node id that driver running on. It will be None only when the job status
  // is PENDING, and this field will not be deleted or modified even if the driver dies
  optional string driver_node_id = 13;
+  // The driver process exit code after driver terminates
+  optional string driver_exit_code = 14;


Same here, why string instead of int

Some test should have failed here, right? Or can a Python int silently be loaded into a string protobuf without any error? (Or is this line of code not currently run in any test?)

It didn't fail any tests before hmm

Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>

rkooo567 · 2023-09-26T21:20:33Z

Btw, we should follow up with this. Adding the exit code itself is not very useful (we should integrate to prod or Ray dashboard)

architkulkarni · 2023-09-26T21:57:39Z

Btw, we should follow up with this. Adding the exit code itself is not very useful (we should integrate to prod or Ray dashboard)

Adding it to the dashboard is tracked here #38142

Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com> Signed-off-by: Victor <vctr.y.m@example.com>

add driver exit code to job status

83cd6c1

Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>

jonathan-anyscale assigned jjyao Sep 14, 2023

jonathan-anyscale marked this pull request as ready for review September 15, 2023 21:59

jonathan-anyscale requested review from wuisawesome, ericl, AmeerHajAli, robertnishihara, pcmoritz, raulchen and a team as code owners September 15, 2023 21:59

jjyao reviewed Sep 20, 2023

View reviewed changes

dashboard/modules/job/common.py Outdated Show resolved Hide resolved

dashboard/modules/job/job_manager.py Outdated Show resolved Hide resolved

dashboard/modules/job/job_manager.py Outdated Show resolved Hide resolved

jonathan-anyscale added 2 commits September 21, 2023 14:16

none exit code for stopped job

5be7b53

Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>

lint

f54566d

Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>

jonathan-anyscale requested a review from jjyao September 21, 2023 23:01

jonathan-anyscale added 4 commits September 21, 2023 16:24

Merge branch 'master' of github.com:ray-project/ray into job_exit_code

403809c

Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>

proto lint

f3d8caa

Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>

clean some codes

4f2f25f

Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>

nit: remove line

649e5fb

Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>

jjyao reviewed Sep 22, 2023

View reviewed changes

dashboard/modules/job/job_manager.py Outdated Show resolved Hide resolved

dashboard/modules/job/tests/test_job_manager.py Outdated Show resolved Hide resolved

rkooo567 self-assigned this Sep 22, 2023

jjyao assigned architkulkarni Sep 22, 2023

fix test

7011006

Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>

architkulkarni approved these changes Sep 22, 2023

View reviewed changes

architkulkarni mentioned this pull request Sep 22, 2023

[jobs] Return status code after job is completed #38142

Open

jonathan-anyscale added 2 commits September 22, 2023 13:47

driver exit code type to int

d0fa6e4

Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>

proto

f29e62d

Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>

jonathan-anyscale requested a review from jjyao September 22, 2023 22:28

jjyao approved these changes Sep 22, 2023

View reviewed changes

jonathan-anyscale changed the title ~~add driver exit code to job info~~ [Job] add driver exit code to job info Sep 22, 2023

jjyao merged commit f2eaea0 into ray-project:master Sep 24, 2023
103 of 108 checks passed

jonathan-anyscale linked an issue Sep 26, 2023 that may be closed by this pull request

[jobs] Return status code after job is completed #38142

Open

simonsays1980 pushed a commit to simonsays1980/ray that referenced this pull request Sep 26, 2023

[Job] add driver exit code to job info (ray-project#39675)

2e8f631

Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com>

vymao pushed a commit to vymao/ray that referenced this pull request Oct 11, 2023

[Job] add driver exit code to job info (ray-project#39675)

7c4cd69

Signed-off-by: Jonathan Nitisastro <jonathancn@anyscale.com> Signed-off-by: Victor <vctr.y.m@example.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Job] add driver exit code to job info #39675

[Job] add driver exit code to job info #39675

jonathan-anyscale commented Sep 14, 2023

jjyao left a comment

architkulkarni left a comment

architkulkarni Sep 22, 2023

jonathan-anyscale Sep 22, 2023

architkulkarni Sep 22, 2023

architkulkarni Sep 22, 2023

architkulkarni Sep 22, 2023

architkulkarni Sep 22, 2023

jonathan-anyscale Sep 22, 2023

rkooo567 commented Sep 26, 2023

architkulkarni commented Sep 26, 2023

[Job] add driver exit code to job info #39675

[Job] add driver exit code to job info #39675

Conversation

jonathan-anyscale commented Sep 14, 2023

Why are these changes needed?

Related issue number

Checks

jjyao left a comment

Choose a reason for hiding this comment

architkulkarni left a comment

Choose a reason for hiding this comment

architkulkarni Sep 22, 2023

Choose a reason for hiding this comment

jonathan-anyscale Sep 22, 2023

Choose a reason for hiding this comment

architkulkarni Sep 22, 2023

Choose a reason for hiding this comment

architkulkarni Sep 22, 2023

Choose a reason for hiding this comment

architkulkarni Sep 22, 2023

Choose a reason for hiding this comment

architkulkarni Sep 22, 2023

Choose a reason for hiding this comment

jonathan-anyscale Sep 22, 2023

Choose a reason for hiding this comment

rkooo567 commented Sep 26, 2023

architkulkarni commented Sep 26, 2023