Skip to content

Conversation

@SeungjinYang
Copy link
Collaborator

For All nodes up + ray cluster healthy. message and transitioning to INIT message, do not add another row into the db if the last event for the cluster already has the same message.

The PR also proposes deleting Starting job execution mostly because this line has little to do with the state of the cluster.

Tested (run the relevant ones):

  • Code formatting: install pre-commit (auto-check on commit) or bash format.sh
  • Any manual or new tests for this PR (please specify below)
  • All smoke tests: /smoke-test (CI) or pytest tests/test_smoke.py (local)
  • Relevant individual tests: /smoke-test -k test_name (CI) or pytest tests/test_smoke.py::test_name (local)
  • Backward compatibility: /quicktest-core (CI) or pytest tests/smoke_tests/test_backward_compat.py (local)

sky/execution.py Outdated
Comment on lines -479 to -483
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's debatable whether this should be deleted, I'm open to keeping it.
Though if we're keeping it we should move it to after job_id definition and include the job id in the event row

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with removing for now. The user will know if their job starts running anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to add the event_type in the call from get_cluster_from_name() and get_clusters_from_history() after rebase on master.

@lloyd-brown
Copy link
Collaborator

Do you want terminate entries for terminated clusters in same PR? If so 02dd13e#diff-db4566a60e392e4076a1a2bc4eaa56e57c4c2a060b23ff98e8b7b71e3e3d35f1L750

@SeungjinYang
Copy link
Collaborator Author

/quicktest-core

Copy link
Collaborator

@lloyd-brown lloyd-brown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@SeungjinYang SeungjinYang merged commit a2da82f into master Aug 11, 2025
16 checks passed
@SeungjinYang SeungjinYang deleted the events-prune branch August 11, 2025 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants