feat: add gift-eval exp replication - #4
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds and documents an experiment workflow to replicate GIFT-Eval results using foundationforecast, including model registry expansion, verification tooling, and job orchestration updates.
Changes:
- Expand GIFT-Eval experiment model registry (with reference slugs, aliases, and CI subset updates) and add verification + replication-table generation utilities.
- Add per-job timing capture (
timing.json) and backfill/rerun helpers for CI and Modal execution. - Adjust model dispatch/validation for TiRex and TimesFM to support GIFT-Eval-specific repo IDs; update docs/examples accordingly.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/models/test_tirex.py | Extends TiRex dispatch tests for GIFT-Eval-specific repo IDs. |
| foundationforecast/models/tirex.py | Updates TiRex v2 detection logic to include TiRex-2-* variants. |
| foundationforecast/models/timesfm.py | Adds allowlist to permit specific non-pytorch TimesFM repo IDs for replication runs. |
| experiments/gift-eval/uv.lock | Switches experiment package source to editable. |
| experiments/gift-eval/src/verify/verify.py | Adds model-level result aggregation and verification utilities plus model filtering helpers. |
| experiments/gift-eval/src/verify/replication_table.py | New module to build/write a replication analysis CSV (metrics + diffs + timing). |
| experiments/gift-eval/src/verify/reference.py | Defines shared metric column constants for verification/reporting. |
| experiments/gift-eval/src/runners/run_verify.py | New CLI runner to verify results and generate replication tables (local/CI modes). |
| experiments/gift-eval/src/runners/run_modal.py | Refactors Modal dispatch, adds S3-based selection for missing results/timing, adds force mode. |
| experiments/gift-eval/src/runners/run_ci.py | Adds local-only “missing timing” rerun mode for CI subset jobs. |
| experiments/gift-eval/src/eval/models.py | Ensures per-model kwargs isolation and defaults alias from reference_slug when unset. |
| experiments/gift-eval/src/eval/jobs.py | Adds timing.json path helper and job filter for missing timing. |
| experiments/gift-eval/src/eval/evaluate.py | Records per-job wall time to timing.json and logs elapsed time. |
| experiments/gift-eval/README.md | Documents verification workflow, replication table semantics, and timing backfill strategies. |
| experiments/gift-eval/Makefile | Adds sync/verify/table targets for experiment workflows. |
| experiments/gift-eval/configs/models.yaml | Expands/normalizes model registry (keys, repo_id mapping, reference slugs, alias defaults). |
| experiments/gift-eval/configs/ci_subset.yaml | Updates CI subset model keys to match new registry keys. |
| docs/examples/ts-foundation-models-comparison-quickstart.ipynb | Updates import list (removes Toto from example import). |
| docs/examples/forecaster-quickstart.ipynb | Formatting-only change (blank line). |
| docs/examples/finetuning.ipynb | Reorders/cleans imports and normalizes typography in markdown/output. |
| docs/examples/anomaly-detection-forecaster-quickstart.ipynb | Formatting-only change (blank line). |
Suppressed comments (1)
foundationforecast/models/timesfm.py:255
- The ValueError message is now misleading: this guard allows repo_ids that do not contain "pytorch" (via the allowlist), but the error text still says "only supports pytorch models" and suggests opening an issue for JAX. Please update the message to reflect the actual acceptance criteria / allowlist so users aren’t confused when a non-"pytorch" repo_id is permitted (or rejected).
if "pytorch" not in repo_id and repo_id not in _GIFT_EVAL_TORCH_REPOS:
raise ValueError(
"TimesFM only supports pytorch models, "
"if you'd like to use jax, please open an issue"
)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 26 changed files in this pull request and generated 4 comments.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
experiments/gift-eval/configs/ci_subset.yaml:1
- The header comment says this is "one GPU job per wrapper class", but the file currently lists a smaller subset. Please update the comment to match what the file actually contains (or add the missing jobs).
# CI subset: one GPU job per wrapper class (+ chronos on m4_hourly).
foundationforecast/models/timesfm.py:255
- The repo_id gate now allows non-
*pytorch*IDs via_GIFT_EVAL_TORCH_REPOS, but the error message still implies the only valid IDs contain "pytorch". Updating the message to describe the actual acceptance criteria will make failures easier to diagnose.
if "pytorch" not in repo_id and repo_id not in _GIFT_EVAL_TORCH_REPOS:
raise ValueError(
"TimesFM only supports pytorch models, "
"if you'd like to use jax, please open an issue"
)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 26 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
foundationforecast/models/timesfm.py:255
- The ValueError message says to "open an issue" for JAX, but the code now explicitly allowlists a repo_id with a "-jax" suffix. This makes the error misleading for users who hit the guard with other non-pytorch repo_ids.
if "pytorch" not in repo_id and repo_id not in _GIFT_EVAL_TORCH_REPOS:
raise ValueError(
"TimesFM only supports pytorch models, "
"if you'd like to use jax, please open an issue"
)
foundationforecast/models/timesfm.py:20
- _GIFT_EVAL_TORCH_REPOS includes a repo_id with a "-jax" suffix, which makes the name misleading and invites accidental removal/incorrect assumptions. Adding a short comment explaining why this allowlist exists (and why a "-jax"-named repo is still acceptable here) would make the intent clear.
_GIFT_EVAL_TORCH_REPOS = (
"google/timesfm-1.0-200m",
"google/timesfm-2.0-500m-jax",
)
experiments/gift-eval/src/verify/verify.py:120
- verify_model() now compares only MASE/CRPS via compare_results(), but it still fails the run if any column in the actual results contains NaNs. That can cause false failures due to unrelated/secondary columns that are not part of the replication criteria.
actual = load_actual_results(model_key, output_root)
if actual.isna().any().any():
raise AssertionError(f"NaN values found in actual results for {model_key!r}")
…pilot/foundationforecast into feat/gift-eval-rep
this pr ads an experiment to replicate gift-eval with foundationforecast