AllenNeuralDynamics / AllenNeuralDynamics/analysis-pipeline-utils
Revisit: hold `code.version` at the latest release instead of enumerating patch commits
- 主要言語
- Python
- スター
- 0
- フォーク
- 1
- PR マージ指標
- 30日以内にマージされた PR はありません
説明
Opening this as the follow-up @tmchartrand suggested in #30. Mostly I want to lay out why I lean toward the looser version, since I don't think I made the failure-mode argument clearly last time.
### **My use case**
On my prototype pipeline, I had roughly two substantial analysis version upgrades across several tens of commits. Most commits were CO tweaks, refactors, or small fixes that didn't change results. So the ratio of "commits I'd need to list" to "versions I actually care about" was something like 50:1.
### Why I think the failure modes are asymmetric
This is my main argument, and it's about what happens when the user forgets, since both designs assume some discipline.
*Holding at the latest release* — if the user forgets to cut a release after a substantial change, the pipeline dispatches nothing new. That's conservative: it saves compute by default, the signal is immediate and visible ("no new jobs launched"), and the fix is one click. Nothing needs cleaning up.
*Enumerating patch commits* — if the user forgets to add a SHA, the version silently falls back to a commit hash and every job looks new. In my case that could be >10K sessions × 8 models ≈ 80K duplicate jobs. There's no warning on that path, so it may not be caught for a while, and the cleanup is manual across both S3 and DocDB.
So one design fails toward doing too little and self-corrects; the other fails toward doing a lot of expensive work that's tedious to undo. Given users are power users either way, I'd rather the default mistake be the cheap one.
**Provenance isn't affected either way.** Worth stating explicitly, since it narrows the disagreement: neither option loses information. The actual commit hash is recorded on `process.notes`, outside `code`, so it never contributes to job-skipping but is always available to trace exactly which code produced a given result.
### What using `PATCH_COMMITS` involves today
I hit some friction actually trying it, which may be worth knowing before we decide:
- It's all-or-nothing — one unlisted commit since the release discards the release version entirely, rather than ignoring just that commit. So the list has to be complete, and appended to before each run.
- It has to be set in both the wrapper *and* the dispatcher, since each reads it from its own environment while both compute the same `code` record. If they diverge, the dispatcher's skip hash and the wrapper's written hash stop matching.
- It also needs `CO_CAPSULE_BRANCH` set to a real branch — with it unset, `branch` defaults to `"HEAD"` and the clone fails before the patch list is consulted (#48 / #53).
- It's not currently mentioned in the README or docstrings, so it's hard to discover.
As far as I can tell that means the patch path hasn't really been exercisable in the default configuration yet — which is partly why I'd hesitate to treat "no complaints so far" as evidence either way. #53 fixes the crash without changing any of the semantics here.
### Proposal
Keep `PATCH_COMMITS` as-is, but make holding-at-latest-release the default, with a warning naming the commit count when there's drift. That's the middle path from #30 and I have it implemented already. See the draft PR https://github.com/AllenNeuralDynamics/analysis-pipeline-utils/pull/58
コントリビューションガイド
このリポジトリのコントリビューションガイドは索引されていません
評価
この issue はまだ評価されていません。