fix(egosuite): validate label report envelope - #364
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b943e7eb3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ( | ||
| not isinstance(schema_version, int) | ||
| or isinstance(schema_version, bool) | ||
| or schema_version != SCHEMA_VERSION |
There was a problem hiding this comment.
When a label report comes from a newer producer with an unknown schema version, this exact-version check raises during pipeline.py module initialization, preventing the pipeline from starting. The repository's forward-compatibility guidance requires unknown values to retain their raw representation in an explicit Unknown* variant, emit a warning, and be ignored gracefully rather than raising, so handle future versions through that path while still rejecting malformed known values.
AGENTS.md reference: AGENTS.md:L1-L1
Useful? React with 👍 / 👎.
kstonekuan
left a comment
kstonekuan
left a comment
There was a problem hiding this comment.
The validation itself is right and I would merge it as it stands. It needs a rebase, and the rebase is not mechanical, so here is what collides before you spend time on it.
#362 merged about an hour ago and rewrote the same loader: load_projected_hand_label_report now returns a ProjectedHandLabelReport and matches labels by HFlow source_uri instead of the basename. Merging current main into your branch auto-merges evaluate.py cleanly, conflicts only in the test file, and then fails five tests. Two different causes:
Four are mechanical. #362's new tests hand-write reports as {"frames": [...]} with no envelope, so your check refuses them before they reach what they are testing:
test_legacy_label_report_rejects_one_basename_for_multiple_sources
test_saved_label_report_matches_same_named_sources_by_canonical_provenance
test_saved_label_report_rejects_missing_or_unrelated_canonical_provenance[both cases]
Actual message: field 'schema_version' has value None; supported value is 1
Adding "schema_version": 1, "label_type": "projected-hand-joints" to those four fixtures is the whole fix, and arguably your check is right to have caught them.
One needs a decision from you. You rewrote test_saved_label_report_selects_exact_frames_for_a_canonical_episode to build its report through write_label_report, which is DoD 6 and the better fixture. On main that test hand-writes a report with no source_uri, so it matches through #362's legacy basename path. Through write_label_report the report now carries a real source_uri, matching switches to the identity path, and the hardcoded {"source_uri": "run-a/episode-123.mcap"} no longer corresponds to what was recorded:
ValueError: label manifest has no frames for source identity 'run-a/episode-123.mcap'
The identity write_label_report records comes from App.source_identity() against HFLOW_DATA_ROOT, so for a tmp_path source it is an absolute path, not run-a/.... Reading the recorded source_uri back out of the written report and passing that is the robust answer, and it keeps your real-envelope fixture. Hardcoding a second identity would just move the coupling.
Nothing above is a criticism of the change. Excluding bool from the schema_version integer check is the detail I look for and most people miss: isinstance(True, int) is True, so without that line {"schema_version": true} would have been accepted as version 1. This repo has been bitten by that more than once.
One thing for next time, and it is the reason this is a request rather than a merge with a fixup from me: pytest -q reporting 11 skipped rather than 6 means it ran without --all-extras. uv sync --locked --all-extras is the documented form, and the difference is the mediapipe and vision tests.
Last thing, an invitation rather than a rule. You have eleven merged PRs here, so the good first issue pool is best left for people arriving after you. The higher-leverage work is the advanced backlog, and #365 was filed today by another contributor from actually running the thing against Egocentric-10K, which is the kind of issue worth more than any single starter fix. #311, #312, #320 and the bucket-backed set (#303, #304, #305) are all open and unassigned.
2 participants
Summary
Fixes #307
Validation
AI assistance disclosure
I used OpenAI Codex to help inspect, implement, and validate this change. I reviewed the final diff and the validation results.