chore: document and package research v1 artifacts - #6
Conversation
|
@codex review this PR. Focus on Research V1 artifact distribution: GitHub Release packaging, teammate download workflow, manifest/checksum safety, docs clarity, and ensuring generated checkpoints/buffers stay out of git while remaining unified for Axis 1-4. |
![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: beafe8e051
ℹ️ 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".
| target = (destination / member.name).resolve() | ||
| if not str(target).startswith(str(destination_resolved)): | ||
| raise ValueError(f"archive member escapes destination: {member.name}") |
There was a problem hiding this comment.
The extraction guard compares str(target).startswith(str(destination_resolved)), which is bypassable for sibling paths that share the same prefix (for example, a resolved target under /tmp/research_v1_extract_xxx_evil still starts with /tmp/research_v1_extract_xxx). In that case a crafted archive member can escape the temp extraction directory and write outside the intended sandbox during teammate downloads. This should use a true path containment check (e.g., target.is_relative_to(destination_resolved) or equivalent) rather than string prefix matching.
Useful? React with 👍 / 👎.
| "gh", | ||
| "release", | ||
| "download", | ||
| release, | ||
| "--pattern", |
There was a problem hiding this comment.
The download command does not pass --repo, so gh release download resolves against whatever repository context the user has locally. In fork-based workflows, the same release tag can exist with different artifacts, and this script will accept those as long as readiness thresholds pass, which breaks the Axis 1–4 requirement to use the shared canonical checkpoint/buffer set. Please pin downloads to the expected repo (or validate manifest["repo"] before copying files) to avoid silent divergence.
Useful? React with 👍 / 👎.
beafe8e to
f4298f8
Compare
|
@codex review this PR again. I addressed both prior findings in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4298f87c0
ℹ️ 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".
| raise FileNotFoundError(f"archive missing artifact: {entry['path']}") | ||
| path.parent.mkdir(parents=True, exist_ok=True) | ||
| shutil.copy2(source, path) | ||
| shutil.copy2(extracted_manifest, manifest_path) |
There was a problem hiding this comment.
When the archive does not contain research_v1_artifact_manifest.json but a local manifest exists, extracted_manifest is reassigned to manifest_path and this line then calls shutil.copy2 with the same source and destination. Python raises SameFileError in that case, so the fallback path always fails even though the files are otherwise valid. This makes the intended “archive missing manifest, use working-directory manifest” recovery path unusable.
Useful? React with 👍 / 👎.
f4298f8 to
a3b0863
Compare
|
@codex review this PR again. I addressed the latest manifest fallback issue in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3b0863043
ℹ️ 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".
| prepare_existing_targets(manifest, args.force) | ||
| for path, entry in manifest_artifact_paths(manifest).items(): | ||
| source = extract_dir / entry["path"] | ||
| if not source.exists(): | ||
| raise FileNotFoundError(f"archive missing artifact: {entry['path']}") |
There was a problem hiding this comment.
When --force is used, the script deletes mismatched local artifacts before confirming the archive actually contains all replacement files. If the release archive is incomplete/corrupt (for example, one manifest entry is missing in the tarball), the run fails at copy time and leaves previously valid local artifacts deleted, which can break subsequent experiment runs. Validate sources first (or stage replacements) before unlinking existing files.
Useful? React with 👍 / 👎.
| download_release(args.release, archive.name) | ||
| if not archive.exists(): | ||
| raise FileNotFoundError(f"archive not found: {archive}") |
There was a problem hiding this comment.
The download call uses archive.name, but later existence is checked against Path(args.archive). If a caller passes --archive with a directory component (e.g. dist/research_v1_foundation_artifacts.tar.gz), gh saves the file in the current directory while the script checks dist/... and fails with archive not found. This makes the --archive path argument unreliable outside the default filename-in-cwd case.
Useful? React with 👍 / 👎.
a3b0863 to
dbd9479
Compare
|
@codex review this PR again. I addressed the latest findings in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbd9479545
ℹ️ 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".
| target = (destination / member.name).resolve() | ||
| if not target.is_relative_to(destination_resolved): | ||
| raise ValueError(f"archive member escapes destination: {member.name}") | ||
| tar.extractall(destination, filter="data") |
There was a problem hiding this comment.
extract_archive() unconditionally calls tar.extractall(destination, filter="data"), but the project supports python>=3.10 and this filter argument is only available in newer 3.10 patch releases; on older 3.10 runtimes the download command will crash with TypeError before any artifact validation runs. This makes the new artifact distribution path unusable for a supported interpreter range unless you gate this by feature detection (for example, checking for tarfile.data_filter) and fall back safely.
Useful? React with 👍 / 👎.
dbd9479 to
a0baa28
Compare
|
@codex review this PR again. I addressed the latest tar extraction compatibility finding in |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
This PR adds tooling and docs for distributing the Research V1 generated foundation artifacts through GitHub Releases instead of committing them to git.
Changes:
scripts/package_research_v1_artifacts.py.scripts/download_research_v1_artifacts.py.Shared artifacts
Release:
Assets:
Extracted expected paths:
Local release validation
python scripts/check_research_v1_ready.py --root runs/research_v1 --checkpoint runs/research_v1/base_pretrain_s42/checkpoints/final.zip --eval-csv runs/research_v1/eval_base_pretrain/eval/heldout_random.csv --buffer runs/research_v1/base_explore_large/buffers/failure_buffer.jsonl --min-failures 1000 --min-episodes 100 --min-success-rate 0.10 --min-route-completion 0.35 --max-timeout-rate 0.95-> PASSpython scripts/package_research_v1_artifacts.py --root runs/research_v1 --output dist/research_v1_foundation_artifacts.tar.gz --release-name research-v1-foundation-v1-> PASStar -tzf dist/research_v1_foundation_artifacts.tar.gz | head -50showed the manifest plus the three expected artifact paths/tmp/fasb_release_download_testdownloaded and extracted the archive from GitHub successfullyArtifact metrics
Base checkpoint eval:
0.440.69979810081919910.56Large buffer:
217821780.04178145087235996{'collision': 855, 'offroad': 992, 'solved': 1, 'timeout_or_hesitation': 239, 'unknown': 91}Source validation
python -m compileall fasb tests scripts examples -qpython -m pytest tests/test_component_validation.py tests/test_failure_buffer.py tests/test_metrics.py tests/test_training_stability_patch.py tests/test_plugin_loading.py tests/test_run_dir.py tests/test_research_plugins.py tests/test_aggregate_results.py tests/test_base_checkpoint_quality.py tests/test_failure_buffer_quality.py tests/test_research_v1_ready.py tests/test_research_v1_artifact_scripts.py -q-> 52 passedpython scripts/validate_components.py --config configs/train/fasb_ppo.yaml-> PASSpython scripts/check_env.py --require-metadrive-> PASSpython scripts/run_e2e_stress.py --clean-runs-> PASSReview focus
Please review: