Fix/148 resolution sets one commit - #248
Conversation
|
Apart from these minor notes, LGTM |
📝 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
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.
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.
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.
cc51475 to
1a524c8
Compare
…rror The push job ran right after `resolve_forecasts` with `exit_on_error=False`, which suppressed the Slack notification as well as the exit, so a failed push to the dataset repo was only visible in the logs. Move the job to the end of `main()` and set `exit_on_error=True`. Exiting there costs nothing since the leaderboards and website have already run, so the failure is reported on Slack without stopping anything else.
`clone_and_push_files` pushed origin and the mirrors inside a single `try`, so the `raise_if_error()` check added earlier in this branch turned a failed Hugging Face push into `sys.exit(1)` and took the whole nightly run with it. Also drop `slack_sdk` and `backoff` from `func-push-resolution-sets`; nothing in the job's import chain uses them.
|
I suggest merging the PR that solves #181 before this one. I'll have the PR up soon. It will bundle up the leaderboard pushes into a single commit. But at the moment the three leaderboard jobs run in parallel and their pushes can reject each other. The |
|
Hmm, I don't see why. If that one goes after this one, you'd just add a few more files to push in the commit right? One overall commit containing resolution files, leaderboards, parity dates, and question difficulty per night? Instead of |
3 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