Repository navigation
Python: fix(lab-gaia): accept local parquet data in _ensure_data - #9340
Manohar Paturi (ManoharPaturi) wants to merge 5 commits into
Conversation
_ensure_data only recognized the old metadata.jsonl layout, so a data dir holding only the parquet files the loader itself prefers was ignored and the runner demanded HF_TOKEN for a fresh download. Accept metadata*.parquet in the same existence check. Fixes microsoft#9337 Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>
GitHub Actions
Active
GitHub Actions
Active
agent-framework-automation
Bot
added
python
GitHub Actions
Active
|
/review |
GitHub Actions
Active
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (2 commit(s)): ddd62b7f05d2, a367690bddab
Model: gpt-5.6-sol
Overview
The change aligns _ensure_data with the parquet glob already used by the loader and adds a real parquet fixture that protects the intended offline path. Existing JSONL handling, dependency packaging, and downstream empty-task failure behavior remain intact. However, filename presence alone also treats an incomplete sharded parquet snapshot as ready, allowing a benchmark to run over a silently truncated task set.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
1 verified finding remained after source verification (1 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/lab/gaia/agent_framework_lab_gaia/gaia.py
| """Ensure GAIA data is available locally.""" | ||
| if self.data_dir.exists() and any(self.data_dir.rglob("metadata.jsonl")): | ||
| if self.data_dir.exists() and ( | ||
| any(self.data_dir.rglob("metadata.jsonl")) or any(self.data_dir.rglob("metadata*.parquet")) |
There was a problem hiding this comment.
This treats the first matching shard as a complete dataset, so an interrupted copy containing only metadata-00000-of-00002.parquet now bypasses snapshot_download. The loader can then read that shard and run a successful-looking benchmark over an incomplete task set. When filenames use the ...-of-N convention, please require all N shards before considering local data ready, while continuing to accept complete parquet-only directories.
…ing local data A partial metadata-XXXXX-of-NNNNN family from an interrupted copy would have counted as ready and run the benchmark over a truncated task set. Group shards by directory and prefix and only treat the snapshot as complete when each group holds all its declared shards. Follow-up to the review on microsoft#9340. Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>
|
Fair point, pushed the follow-up: _ensure_data now groups metadata-XXXXX-of-NNNNN files by directory and prefix and only treats the snapshot as ready when every group holds all its declared shards. A partial family forces the normal download path again, and the test covers both halves of that. |
… fix/gaia-parquet-ensure-data
GitHub Actions
Active
|
/review |
There was a problem hiding this comment.
MAF Automated Review — Iteration 2
Result: No findings
Scope: 2 net-new commit(s): 47891ef94bfe, d172e602bca8
Model: gpt-5.6-sol
Overview
The incremental patch replaces the broad parquet-existence check with grouped shard counting and directly tests the previously reported 00000-of-00002 interruption case. Grouping by parent and prefix prevents shards from separate splits from satisfying one another, and empty parquet directories still fall through to the existing download path. Residual malformed-index and mixed-layout cases remain facets of the already unresolved shard-completeness thread rather than new publishable findings in this iteration.
Reviewed the supplied incremental change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.
Eduard van Valkenburg (eavanvalkenburg)
left a comment
Eduard van Valkenburg (eavanvalkenburg)
left a comment
There was a problem hiding this comment.
Manohar Paturi (@ManoharPaturi) Two issues remain on the current head:
python/packages/lab/gaia/agent_framework_lab_gaia/gaia.py:282:_has_complete_parquet_snapshot()returnsTrueas soon as it sees any unshardedmetadata*.parquet. If that directory also contains an incomplete sharded family, validation stops early even though_load_gaia_local()later reads all matching parquet files, so a mixed/incomplete task set can be accepted. Please continue scanning and require every sharded family present to be complete before treating the directory as ready.python/packages/lab/gaia/tests/test_gaia.py:85:test_incomplete_sharded_parquet_snapshot_forces_downloadassumesHF_TOKENis absent. In a developer or CI environment where it is set, the test can perform a realsnapshot_downloadinstead of asserting the intended branch. Please clear the environment variable withmonkeypatch.delenv("HF_TOKEN", raising=False)or mock the download path so the test is hermetic.
An unsharded metadata parquet made the completeness check return early, so a directory mixing a plain file with an incomplete sharded family was treated as ready. Keep scanning after unsharded files and let any incomplete family disqualify the directory. Also clear HF_TOKEN in the incomplete snapshot test so it cannot attempt a real download. Follow-up to the review on microsoft#9340. Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>
GitHub Actions
Active
|
a16b33d: the completeness scan no longer returns on the first unsharded file, it records it and keeps scanning, and any incomplete sharded family now disqualifies the directory even when unsharded files sit beside it. Added a mixed case test (incomplete family plus a plain metadata-extra.parquet still forces the download path). The incomplete snapshot test also clears HF_TOKEN with monkeypatch now, so it stays hermetic. |
Eduard van Valkenburg (eavanvalkenburg)
left a comment
Eduard van Valkenburg (eavanvalkenburg)
left a comment
There was a problem hiding this comment.
Manohar Paturi (@ManoharPaturi) _ensure_data() still returns successfully whenever any metadata.jsonl exists, without checking an incomplete metadata*.parquet shard family in the same directory. _load_gaia_local() prefers parquet and falls back to JSONL only when no parquet tasks load, so one valid partial shard can still produce a silently truncated benchmark run. Please require a complete parquet snapshot whenever any parquet files are present, or explicitly ignore incomplete parquet and deliberately fall back to JSONL, with a mixed-layout regression.
Motivation & Context
_load_gaia_localprefers parquet files (the comment calls it the new format, validation split first) butGAIA._ensure_dataonly treated a data dir as ready when it containedmetadata.jsonl. Pointingdata_dirat a parquet-only mirror was ignored and the runner demanded HF_TOKEN for a fresh download even though the local data was fully usable. Issue #9337 has the repro.Description & Review Guide
_ensure_datanow also acceptsmetadata*.parquetin its existence check, matching the glob the loader itself uses.Related Issue
Fixes #9337
AI Assistance
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.