Fix/148, 181 resolution and leaderboard sets to one commit - #248
Fix/148, 181 resolution and leaderboard sets to one commit#248oskariluoma wants to merge 2 commits into
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
cc51475 to
1a524c8
Compare
|
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 |
6cec2fe to
2ae1894
Compare
oskariluoma
changed the title
|
Fine PR, solves the problem it sets out but I catch myself asking if these really are problems we should be solving at all, @houtanb . The whole GCP-tied architecture remains strange to me, tests are still lacking and I am nervous about every line. There's a ton of assumptions built on top of each other, and all I want to do is just point the code to some folder (on GCS or local) and have all the code just work from whatever state the files are in - each stage must encode exactly what it expects, and test must be the encoding of these expectations. This is obviously not the case here as every stage is very finnicky, local runs/test etc are borderline unrunnable etc. We've talked about this before but I feel like we're doing a lot of patching these days. Not immediately actionable any of this I guess as such a refactor would be significant amount of work*. This codebase currently is worthwhile because it's been battle-tested a decent bit, which is impressive. *although, tbh, as long as we get the tests right, agents can probably execute the majority of it and shadow parity runs/code review over the course of a few weeks can verify correctness/architecture --- even if minor bugs slip through after all that, at least will be debugging/patching something more robust. |
|
@nikbpetrov indeed that's why you're working on the refactor and a test suite! It takes time because we're doing this on a public-facing system so we're going slowly to be confident in changes, but the end goal, depending on tradeoffs, is a more reliable system that will allow us to move more quickly. All ideas are welcome. |
The resolve and leaderboard jobs run in parallel and each pushed its own files to the dataset repo, racing on the remote. They now only write to the bucket; a new `func-push-datasets-to-git` job collects everything and pushes it in a single commit at the end of the nightly run. Parity dates and question fixed effects were never in the dataset repo at all. They now land in `datasets/parity_dates/` and `datasets/question_fixed_effects/`. The resolution sets and the leaderboard html & csv keep their existing paths. The leaderboard wrote a new dated question-fixed-effects file every night and the website served the whole pile. It now writes one file per leaderboard, overwritten nightly, and the history is tracked in the dataset repo instead, so the website page is replaced by a link to the repo from the datasets page. `clone_and_push_files` now skips empty commits, surfaces a push to origin that the remote rejected (GitPython does not raise on this) and tolerates a failing mirror push. Closes forecastingresearch#181 Closes forecastingresearch#148
… the SSH key is unset A failed mirror push was only logged. It now also sends a Slack message. `clone()` returns None when `API_GITHUB_SSH_ID_RSA` is not set, so the tuple unpack in `clone_and_push_files` raised `TypeError` instead of the intended soft skip. It now returns early, before cloning. `helpers/git.py` now imports `helpers/slack.py`, so `slack_sdk` is added to the requirements of the three deployments that use `helpers.git` and were missing it.
25ab688 to
25689ac
Compare
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