Conversation
HOTFIX: update documentation and fix edge case error in arch determination
Reviewer's GuideRefines the documentation for the signed batch size and tail-handling protocols, fixes architecture normalization and supported-arch matching in frun scripts, and corrects an environment variable name in the flags documentation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The change from
ARCH="${ARCH0//_v/-v}"toARCH="${ARCH0//-v/_v}"in_forkrun_get_archflips the normalization direction forx86_64[-_]v[2-4]; double-check this matches the actualARCH0formats produced elsewhere so we don’t end up emitting mixed-v/_vvariants. - Dropping
armv7from thecasebranch and correctingppcle64→ppc64lechanges howarmv7is handled (it now falls through to the default); if that’s intentional, consider adding a brief comment or a separate branch so it’s clear we no longer treatarmv7like the other listed arches.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The change from `ARCH="${ARCH0//_v/-v}"` to `ARCH="${ARCH0//-v/_v}"` in `_forkrun_get_arch` flips the normalization direction for `x86_64[-_]v[2-4]`; double-check this matches the actual `ARCH0` formats produced elsewhere so we don’t end up emitting mixed `-v`/`_v` variants.
- Dropping `armv7` from the `case` branch and correcting `ppcle64` → `ppc64le` changes how `armv7` is handled (it now falls through to the default); if that’s intentional, consider adding a brief comment or a separate branch so it’s clear we no longer treat `armv7` like the other listed arches.
## Individual Comments
### Comment 1
<location path="frun.bash" line_range="1207" />
<code_context>
ARCH='x86_64_v2'
fi
;;
- aarch64|armv7|riscv64|s390x|ppcle64)
+ aarch64|riscv64|s390x|ppc64le)
ARCH="$ARCH0"
;;
</code_context>
<issue_to_address>
**question (bug_risk):** Dropping `armv7` from the handled architectures changes behavior on 32-bit ARM; confirm this is intentional and reflected elsewhere.
`armv7` previously mapped to `ARCH="$ARCH0"` but will now hit the default case in `_forkrun_get_arch`, which may change behavior or cause errors on 32‑bit ARM. If `armv7` is now unsupported, please make that explicit in user-facing errors or selection logic. If it should still be supported, consider keeping it in this case pattern or adding a dedicated branch.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request corrects documentation regarding forkrun flags and updates the signed batch size protocol invariants to include hysteresis and fast-path routing. It also fixes architecture string replacements and supported architectures in the bash scripts. The reviewer points out that maintaining identical duplicate scripts across multiple files (frun.bash, frun.nob64.bash, and frun.nob64.bash.txt) increases maintenance overhead and suggests using symlinks or a build step to keep them synchronized.
| case "$ARCH0" in | ||
| x86_64[-_]v[2-4]) | ||
| ARCH="${ARCH0//_v/-v}" | ||
| ARCH="${ARCH0//-v/_v}" |
There was a problem hiding this comment.
The changes in this file are identical to those in frun.bash and ring_loadables/frun.nob64.bash.txt. Maintaining multiple duplicate copies of the same script increases maintenance overhead and the risk of files drifting out of sync. It would be highly beneficial to use symlinks, source a shared script, or use a build/generation step to keep these files synchronized automatically.
There was a problem hiding this comment.
ring_loadables/frun.nob64.bash.txt is a copy of frun.bash with the base64 embeddings removed. it is generated using ring_loadables/remove_frun_base64.bash. its purpose is to allow for easier AI review of frun.bash without wasting 100's of thousands of tokens on meaningless embedded base64 strings
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee72a7fea3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * May update batch size at any time. | ||
| * Must **always** publish negative values (`-abs(N)`). | ||
| * Must never publish a positive batch size. | ||
| * Must publish a **negative** value (`-N`) ONLY if the new target is at least double the absolute value of the current batch size (`N >= 2 * current`). |
There was a problem hiding this comment.
Update the invariant to use a strict > 2x threshold instead of >= 2x. The runtime publisher in ring_loadables/forkrun_ring.c (PUBLISH_BATCH_SIZE) emits a negative size only when new_L > abs(current)*2, so an exact 2x increase stays on the positive fast path. Documenting >= here will cause incorrect operator expectations and invalid invariant checks around boundary-sized ramps.
Useful? React with 👍 / 👎.
| * If CAS fails (scanner changed policy), re-evaluate under new policy. | ||
| * Observe the published batch size. | ||
| * If positive (`> 0`): Bypass all speculative arithmetic and claim exactly 1 ring slot. | ||
| * If negative (`< 0`): Enter the speculative slow path to calculate how many older slots satisfy the new magnitude. |
There was a problem hiding this comment.
This line states that negative batch sizes make workers compute multi-slot speculative claims, but the current worker claim path in ring_loadables/forkrun_ring.c initializes claim_count = 1 and does not derive multi-claim count from sbatch < 0. Keeping this statement in the invariants will mislead debugging/tuning and future changes that rely on documented worker behavior.
Useful? React with 👍 / 👎.
1 participant
Summary by Sourcery
Refine batch size protocol documentation and correct forkrun architecture and flag handling.
Bug Fixes:
Enhancements:
Documentation: