[core][state] preserve StateSchema column order in filter_fields - #65052
[core][state] preserve StateSchema column order in filter_fields#65052KuongB wants to merge 2 commits into
Conversation
Signed-off-by: KuongB <ttcuong23@clc.fitus.edu.vn>
There was a problem hiding this comment.
Code Review
This pull request updates the filter_fields function in python/ray/util/state/common.py to use state_dataclass.list_columns(detail=detail) and adds a unit test to verify that filter_fields preserves the schema's column order. The reviewer suggested a valuable improvement to the new test: reversing the order of the input dictionary's keys to ensure the test robustly verifies that the schema's column order is enforced regardless of the input dictionary's key order.
There was a problem hiding this comment.
Pull request overview
This PR makes State API response payloads deterministic by ensuring filter_fields() emits keys in the canonical StateSchema column order (dataclass field order) instead of iterating over unordered set-based column collections.
Changes:
- Update
filter_fields()to iterate viaStateSchema.list_columns(detail=...)to preserve schema-defined ordering. - Add a unit test asserting
filter_fields()preservesStateSchemacolumn order for bothdetail=Trueanddetail=False.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| python/ray/util/state/common.py | Switches filter_fields() to iterate ordered columns from StateSchema.list_columns() for deterministic API dict key order. |
| python/ray/tests/test_state_api.py | Adds a regression test verifying filter_fields() preserves schema column order. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: KuongB <ttcuong23@clc.fitus.edu.vn> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Tran Tien Cuong <147846333+KuongB@users.noreply.github.com> Signed-off-by: KuongB <ttcuong23@clc.fitus.edu.vn>
62fc00c to
02fba73
Compare
martinlhw
left a comment
There was a problem hiding this comment.
Hey thanks for the contribution. If the CLI is correct today, could you elaborate on why this fix is needed?
Hi @martinlhw, thanks for taking a look. The CLI is correct today — this isn't a user-visible bug fix. The case for it is consistency: That inconsistency is what makes #30805 confusing to reason about: the CLI happens to be correct not because the ordering is enforced, but because To be clear on scope: JSON objects are unordered by spec, and I'm not aware of anything in-tree that breaks today. The value is having one canonical order that every consumer honours, rather than one that happens to survive by accident. |
One correction to my own comment above, now that I've looked at the history: item 2 was deliberately fixed by #32353, which prepended The narrower point stands: Incidentally, #32353's review thread is a small argument for the consistency approach . This PR is the same idea: while tracing that issue I found the one place still inconsistent with it, and patched it. |
Description
Item 2 of #30805 asks that
task_idappear first inray list tasks --detail, so that tasks are easier to track.Checking against current master, that symptom no longer reproduces:
task_idis already the first field of theTaskStatedataclass, and all CLI output paths (table, YAML,--format json) follow the dataclass field order. The CLI is correct today.The root cause behind it, however, is still live one layer down.
filter_fields()inpython/ray/util/state/common.pybuilds the payload that the State API server returns, and it iterated overcolumns()/base_columns()— both of which return aset. Iterating aset[str]yields an order that depends on the per-process string hash seed, so the key order of/api/v0/*responses is non-deterministic and changes on every restart.This is invisible from the CLI because
StateApiClient.list()rebuilds each dict into aTaskStatedataclass, which restores the schema order. Anything consuming the HTTP API directly — the dashboard, external tooling — sees the raw, shuffledorder.
StateSchema.list_columns()already defines the canonical order, so this PR simply uses it infilter_fields(). It selects the same set of columns as before, only ordered.Reproduction (before this change)
base_columns()returns a different order in every process:task_idlands at position 3, 4, and 1 across three consecutive runs. That intermittency is likely why this went unnoticed for so long.After
The same three commands, with this change applied.
Identical every run, and matching the
TaskStatefield declaration order.Related issues
Addresses the second item in #30805 ("the ordering of the results ... Task Id should be the first"), by fixing the underlying cause rather than reordering columns at the presentation layer.
Supersedes #64283 — per @edoakes' review comment there, the column order is defined by
StateSchemainstead of being special-cased instate_cli.py.