Conversation
develop and main are both push- and pull_request-trigger branches; a commit landing on develop while a develop->main release PR is open fires both events for the same SHA (surfaced by PR #320, where the push- triggered run hit a transient apt.llvm.org outage independently of the pull_request-triggered run that had already passed). Add a workflow-level concurrency group keyed on the actual commit SHA (pull_request head SHA, else push SHA) with cancel-in-progress so the redundant run is cancelled instead of wasting CI minutes and doubling exposure to flaky externals.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change introduces chunked quantized tensor conversion and accumulation, controlled execute-op output aliasing, optimizer in-place contracts, expanded boundary tests, updated macOS stack watermark baselines, and CI gating for redundant push runs. ChangesTensor conversion and accumulation
Execute-op integration
CI and watermark maintenance
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…them The concurrency-cancel fix from #323 dedupes push+pull_request duplicate runs on the same commit, but a cancelled run's jobs surface as failed-looking red X's in the PR checks list (confirmed live on #324: run 29046387852 was cancelled mid-flight, its 6 jobs all showing "fail" in `gh pr checks`) -- not acceptable even though it doesn't block merging. Replace it with a `gate` job: for push events, it checks (via `gh pr list`) whether the pushed commit is already the head of an open PR. If so, that PR's own pull_request-triggered run already provides full coverage, so every other job's `if: needs.gate.outputs.proceed == 'true'` skips them outright -- before any real job starts, showing a clean neutral "skipped" instead of cancelled/failed. pull_request- triggered runs always proceed (gate is a no-op for them), and ordinary pushes with no open PR are unaffected.
2 tasks
Independent review (fable) of #325 found two real bugs in the gate job: - run: steps use bash -e by default; an unhandled gh pr list failure (API hiccup) would abort the script before `proceed` is ever written, failing the gate job itself -- the exact red-X-on-transient-flake outcome #325 was meant to eliminate, now just relocated to api.github.com instead of apt.llvm.org. `|| dup=0` makes a lookup failure fail open (full run proceeds) instead of failing the gate. - `gh pr list` defaults to --limit 30; past that many concurrently open PRs, a real duplicate could go undetected (fails open the same way, just silently -- harmless but worth closing). --limit 100 covers this repo's realistic ceiling.
…ing + OUT_WRITE aliasing (#296 Stage 1) - executeOp prologue: one summed scratch VLA sized to the operands that actually convert (zero bytes when all dtypes match the arithmetic), replacing the unconditional scratch[3][maxElems*4] - rawData aliasing: FLOAT32-arithmetic -> FLOAT32-target OUT_WRITE kernels write the target directly; gated on no post-prologue operand aliasing the target unless the new opSpec_t.writesInPlaceSafe (elementwise contract) is set; never for OUT_ACC (staging load-bearing) or non-FLOAT32 targets (width-restore load-bearing) - the three SGD update kernels opt in (verified elementwise) - stack-watermark budgets recalibrated to measured post-change peaks: float 99560 -> 27328 B (peak moved to conv3 weight-grad OUT_ACC), sym 149008 -> 100080 B (repack chain remains for Stage 2); pytest fixtures updated to the new measured values Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…— fixed 256-element streaming replaces tensor-sized VLAs (#296 Stage 2) - chunk infrastructure: ODT_CONVERSION_CHUNK_ELEMS (256, multiple of 8 so packed chunk starts stay byte-aligned), packChunkGuarded (abort-on-overflow, same message/exit as the old fit guard), unpack{Sign,Zero}ExtendChunk, public dequantChunkToFloat, quantizeFloatToAsym split into grid + emit - every element-local TensorConversion cell rewritten as a chunk stream: pack family, four two-pass derived-grid cross-cells, nine unpack/dequant cells (two became staging-free); grid scans never round, exactly one roundByMode per element in element order -- SR-RNG streams bit-identical - streamed accumulate engines (FixedGrid/Rescale/AsymRescale via incSrc_t; float* signatures unchanged) + tensor-typed epilogue entry points; new accumulateSymInt32IntoSymInt32Rescale reproduces Strategy A bit-for-bit (pinned against the live Add.c implementation); executeOp Phase-4 staging VLAs and incrementAsFloatView deleted; the float->SYM_INT32 dynamic-rescale arm stays byte-identical as the documented residual (spec §5) - packFitGuarded deleted (last caller gone); TensorConversion compiles clean under -Werror=vla - stack-watermark budgets recalibrated to measured Stage-2 peaks: float 27768 B (+440 B chunk-buffer frames), sym 51968 B (repack chain eliminated; was 100080 post-Stage-1, 149008 pre-#296) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@src/tensor/TensorConversion.c`:
- Around line 817-830: The in-place accumulation path must preserve the
increment’s original quantization metadata when source and target alias. Update
accumulateTensorIntoSymRescale and the ASYM equivalent to snapshot the increment
scale, zero-point, and related configuration before modifying target, then make
incSrcChunk use that snapshot during phase B; alternatively reject aliasing
explicitly in the public wrappers.
- Around line 107-112: Update dequantChunkToFloat to validate the requested
range against the source tensor before accessing data: ensure elemOffset is
within the tensor and elemOffset + count does not exceed its element count,
while retaining the existing chunk-size and alignment checks. Reject invalid
ranges through the existing PRINT_ERROR/exit path before any memcpy or unpacking
occurs.
- Around line 182-185: Update quantizeFloatToAsym to handle n == 0 before
calling findMinFloat or findMaxFloat, initializing the output quantization
configuration and returning safely without reading input values; keep the
existing min/max conversion path for non-empty tensors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d61defad-ceac-4d93-8edd-4129c66feb99
📒 Files selected for processing (10)
.github/workflows/ci.ymlexamples/_shared/check_stack_watermark.pypython/tests/test_check_stack_watermark.pysrc/arithmetic/ExecuteOp.csrc/arithmetic/include/ExecuteOp.hsrc/optimizer/Sgd.csrc/tensor/TensorConversion.csrc/tensor/include/TensorConversion.htest/unit/arithmetic/UnitTestExecuteOp.ctest/unit/tensor/UnitTestTensorConversion.c
✅ Files skipped from review due to trivial changes (1)
- python/tests/test_check_stack_watermark.py
… quantizeFloatToAsym n==0 no-op, reject self-aliased accumulate increments (release-review PR #324)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
develop and main are both push- and pull_request-trigger branches; a commit landing on develop while a develop->main release PR is open fires both events for the same SHA (surfaced by PR #320, where the push- triggered run hit a transient apt.llvm.org outage independently of the pull_request-triggered run that had already passed). Add a workflow-level concurrency group keyed on the actual commit SHA (pull_request head SHA, else push SHA) with cancel-in-progress so the redundant run is cancelled instead of wasting CI minutes and doubling exposure to flaky externals.