Skip to content

[Core] Fix get_last_job() not actually sorting in test_task_events_2#62616

Open
Yicheng-Lu-llll wants to merge 3 commits intomasterfrom
Yicheng-Lu-llll-fix-sort
Open

[Core] Fix get_last_job() not actually sorting in test_task_events_2#62616
Yicheng-Lu-llll wants to merge 3 commits intomasterfrom
Yicheng-Lu-llll-fix-sort

Conversation

@Yicheng-Lu-llll
Copy link
Copy Markdown
Member

Description

sorted() returns a new sorted list instead of sorting in place. As a result, get_last_job() was returning an arbitrary job instead of the latest one.

Signed-off-by: Yicheng-Lu-llll <51814063+Yicheng-Lu-llll@users.noreply.github.com>
@Yicheng-Lu-llll Yicheng-Lu-llll requested a review from a team as a code owner April 14, 2026 18:56
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the get_last_job helper function in the Ray task events tests to use max() for retrieving the latest job ID. A review comment correctly identifies that the implementation uses inconsistent access patterns (dictionary vs. attribute) which would likely cause an AttributeError, and suggests handling empty job lists to avoid a ValueError.

@Yicheng-Lu-llll Yicheng-Lu-llll added go add ONLY when ready to merge, run all tests core Issues that should be addressed in Ray Core labels Apr 14, 2026
Signed-off-by: Yicheng-Lu-llll <51814063+Yicheng-Lu-llll@users.noreply.github.com>
Signed-off-by: Yicheng-Lu-llll <51814063+Yicheng-Lu-llll@users.noreply.github.com>
@edoakes
Copy link
Copy Markdown
Collaborator

edoakes commented Apr 14, 2026

oops!

@edoakes edoakes enabled auto-merge (squash) April 14, 2026 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Issues that should be addressed in Ray Core go add ONLY when ready to merge, run all tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants