Fix/148 resolution sets one commit#248
Conversation
New single-task Cloud Run job `func-push-resolution-sets` gathers all resolution sets from the bucket and pushes them in one commit, fixing the race condition from parallel resolve tasks pushing independently. Still needs end-to-end testing in a GCP dev project. Refs forecastingresearch#148
clone_and_push_files now returns whether it pushed, and push_all_resolution_sets only logs the push count when a commit was actually made. Avoids a misleading 'Pushed N resolution sets' log on nights where the empty-commit skip applied.
…sets-one-commit # Conflicts: # src/orchestration/_io.py
Upstream moved orchestration jobs to the shared orchestration_upload.mk staging macro and made utils a pip dependency (fri-utils). Update the resolution-set push job's Makefile to match instead of copying the removed utils submodule by hand.
| shutil.copy(source, full_destination_path, follow_symlinks=False) | ||
| repo.index.add([destination]) | ||
|
|
||
| # Skip empty commits: nothing to push if the staged files match HEAD. |
There was a problem hiding this comment.
what happens if a mirror fails but git succeeds? mirror would never get the update on same-day rerun?
There was a problem hiding this comment.
Yeah you're right about that. My plan for the fix: only make a commit when something actually changed, but always push to both origin and the mirror anyway. Git only sends what's missing, so if a remote is already up to date the push just does nothing, and if the mirror fell behind it catches up on the next run. Would that work?
There was a problem hiding this comment.
you're actually right about git, but then why would you even download from git then? git will indeed only push changes so even if older files changed somehow, those changes would be pushed correctly
no idea about mirrors, probably @houtanb can better advise; mirrors is written as a generic one so not sure what that could capture in theory, and also not sure if that's how HF works either
There was a problem hiding this comment.
The diff check and the push do different jobs: git push only compares commit pointers, not file contents.
Two scenarios:
A: content unchanged. We clone origin, so HEAD == origin's HEAD. Diff has no changes => index.commit() is skipped => no new commit. origin.push() sees the remote already has the same commit HEAD => transfers nothing. But if yesterday's mirror push failed, the mirror is behind => the mirror push sends it the missing commits and it catches up. That's the fix for the original concern.
B: content changed. Diff has changes => new commit with index.commit() => both pushes send it, as expected.
Why not just always commit and let push sort it out? GitPython's index.commit() creates a commit even with an identical tree and origin.push() pushes it as the commit is new. Without the diff check, the dataset repo would gain an empty commit every night.
|
Apart from these minor notes, LGTM |
Push origin and mirrors on every run so a mirror that fell behind on an earlier partial failure catches up, while still skipping empty commits. Check push results with `raise_if_error()`, which GitPython does not raise on by default, so a rejected push now fails the job instead of being reported as success.
📝 WalkthroughWalkthroughThis PR adds a dedicated push-resolution-sets pipeline step. ChangesPush Resolution Sets Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Manager as Nightly Manager
participant Worker
participant CloudRunJob as func-push-resolution-sets
participant IO as orchestration._io
participant Git as git helper
Manager->>Worker: call_worker(resolve_forecasts)
Worker-->>Manager: block_and_check_job_result
Manager->>Worker: call_worker(push_resolution_sets)
Worker->>CloudRunJob: run func-push-resolution-sets
CloudRunJob->>IO: push_all_resolution_sets()
IO->>IO: list & download GCS resolution sets
IO->>Git: clone_and_push_files(files, message)
Git->>Git: detect changes, commit if needed
Git->>Git: push to origin and mirrors
Git-->>IO: return has_changes (bool)
IO-->>CloudRunJob: log pushed count
CloudRunJob-->>Worker: job complete
Worker-->>Manager: block_and_check_job_result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/orchestration/_io.py (1)
264-308: 🚀 Performance & Scalability | 🔵 TrivialFull resync of all historical resolution sets on every run.
push_all_resolution_sets()lists, downloads, and restages every resolution-set file under the prefix on each invocation, not just newly uploaded ones. This is presumably intentional (self-healing if a prior push/mirror failed), but as the corpus grows this means unbounded, ever-increasing GCS I/O and git index work per nightly run for what is typically a single new file.Worth keeping an eye on job duration/cost over time; if it becomes a bottleneck, consider only fetching files newer than the last successful push (e.g. tracked via a marker/manifest) while still falling back to a full resync periodically for recovery.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/orchestration/_io.py` around lines 264 - 308, `push_all_resolution_sets()` currently re-downloads and re-commits every resolution set on each run, which will keep increasing GCS and git work as the dataset grows. Update this flow to avoid a full resync on every invocation by tracking the last successful push (for example with a marker or manifest) and only fetching/restaging newer files, while preserving a periodic or fallback full resync for recovery. Use the existing `push_all_resolution_sets`, `gcp.storage.list_with_prefix`, and `git.clone_and_push_files` flow as the place to implement the incremental behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/orchestration/_io.py`:
- Around line 264-308: `push_all_resolution_sets()` currently re-downloads and
re-commits every resolution set on each run, which will keep increasing GCS and
git work as the dataset grows. Update this flow to avoid a full resync on every
invocation by tracking the last successful push (for example with a marker or
manifest) and only fetching/restaging newer files, while preserving a periodic
or fallback full resync for recovery. Use the existing
`push_all_resolution_sets`, `gcp.storage.list_with_prefix`, and
`git.clone_and_push_files` flow as the place to implement the incremental
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e273aa85-329e-48ac-b0bb-5d16bd97c11d
📒 Files selected for processing (8)
Makefilesrc/helpers/git.pysrc/nightly_update_workflow/manager/main.pysrc/nightly_update_workflow/worker/main.pysrc/orchestration/_io.pysrc/orchestration/func_push_resolution_sets/Makefilesrc/orchestration/func_push_resolution_sets/main.pysrc/orchestration/func_push_resolution_sets/requirements.txt
2 participants
src/orchestration/_io.py— upload_resolution_set now only uploads to the bucket (no git push). Added push_all_resolution_sets() that gathers all resolution sets from the bucket and pushes them in one commit.src/orchestration/func_push_resolution_sets/— new Cloud Run job (func-push-resolution-sets) that runs push_all_resolution_sets() as a single task.src/nightly_update_workflow/manager/main.py— runs the push job after resolve finishes, before leaderboards.src/nightly_update_workflow/worker/main.py— registers the new job.Makefile— adds the push-resolution-sets target and wires it into resolve.src/helpers/git.py— clone_and_push_files skips empty commits and returns whether it pushed (so no false "pushed" log when nothing changed).Summary by CodeRabbit
New Features
Bug Fixes