ci: split OOMing model shards finer + cover ~780 never-run tests (Document AI, Finance, integration suite) - #1540
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
|
Warning Review limit reached
More reviews will be available in 47 minutes and 37 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughRefactors the SonarCloud CI test matrix to split coarse model-family shards into finer letter-range shards, adds new integration and top-level shards, updates related comments, and replaces the ChangesTest Sharding Refinement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…M-Z, OOM cause) PR #1540's first commit added ModelFamily-NeuralNetworks A-L / M-Z shards to replace the old monolithic Unit-08e catch-all, but forgot to remove 08e itself. 08e was filtering on the same ModelFamilyTests.NeuralNetworks namespace as A-L+M-Z and only excluded 5 unit-test classes that already have their own 08a-08d dedicated shards. Net effect: every paper-scale model in the ModelFamily-NN scaffold was being instantiated + trained twice per CI run (once under A-L/M-Z, once under 08e). The second pass started after Adam state was already allocated and pushed total resident set over the runner's 16 GB envelope, triggering the OOM that's been blocking PR #1540's CI. Drop 08e and its $heavyShards / disk-cleanup comment references. A-L and M-Z (model-class-letter-bucketed and serialized via xunit.runner.json on the matching $heavyShards rows) provide complete non-redundant coverage of the same ~85 NN model-family test classes.
|
Deployment failed with the following error: Learn More: https://vercel.com/franklins-projects-02a0b5a0?upgradeToPro=build-rate-limit |
|
Deployment failed with the following error: Learn More: https://vercel.com/franklins-projects-02a0b5a0?upgradeToPro=build-rate-limit |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/sonarcloud.yml (1)
678-685:⚠️ Potential issue | 🟠 MajorMake the PowerShell JSON rewrite resilient: use
Add-Member -ForceforparallelizeTestCollections/maxParallelThreadsDirectly assigning
$cfg.parallelizeTestCollections/$cfg.maxParallelThreadsafterConvertFrom-Jsoncan throw on aPSCustomObjectif those keys are missing in the workflow-producedxunit.runner.json, which would break the intended heavy-shard OOM mitigation. The script already usesAdd-Member -ForceforpreEnumerateTheories, so extend that same safe pattern to these two keys as well.Proposed fix
$cfg = Get-Content $runnerJson -Raw | ConvertFrom-Json - $cfg.parallelizeTestCollections = $false - $cfg.maxParallelThreads = 1 + $cfg | Add-Member -NotePropertyName parallelizeTestCollections -NotePropertyValue $false -Force + $cfg | Add-Member -NotePropertyName maxParallelThreads -NotePropertyValue 1 -Force # Don't materialize every [Theory]'s data rows at discovery time — # the assembly discovers ~64k cases and pre-enumeration inflates the # retained-case baseline before a single test even runs. $cfg | Add-Member -NotePropertyName preEnumerateTheories -NotePropertyValue $false -Force🤖 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 @.github/workflows/sonarcloud.yml around lines 678 - 685, The script mutates the PSCustomObject $cfg by assigning $cfg.parallelizeTestCollections and $cfg.maxParallelThreads directly which can fail if those properties don't exist; change those two assignments to use Add-Member -NotePropertyName parallelizeTestCollections / -NotePropertyName maxParallelThreads with -NotePropertyValue (false / 1) and -Force (matching how preEnumerateTheories is added) so the JSON rewrite is resilient when keys are absent, then ConvertTo-Json and Set-Content as before.
🤖 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 @.github/workflows/sonarcloud.yml:
- Around line 526-527: Update the troubleshooting comment text that still
references the old shard group names (e.g., "Diffusion S-Z", "Diffusion J-R",
"Generated", "ModelFamily-NN") so it matches the current matrix naming; replace
those old names with the new shard names used in the matrix ("Diffusion S",
"Diffusion T-Z", "Diffusion J-M", "Diffusion N-R", and "Generated Layers
A-M/N-Z") wherever they appear in the sonarcloud workflow comments (the blocks
around the current coverage/dump guidance). Ensure all occurrences in the
workflow comments are synchronized so runbook instructions point to the correct
shard sets.
---
Outside diff comments:
In @.github/workflows/sonarcloud.yml:
- Around line 678-685: The script mutates the PSCustomObject $cfg by assigning
$cfg.parallelizeTestCollections and $cfg.maxParallelThreads directly which can
fail if those properties don't exist; change those two assignments to use
Add-Member -NotePropertyName parallelizeTestCollections / -NotePropertyName
maxParallelThreads with -NotePropertyValue (false / 1) and -Force (matching how
preEnumerateTheories is added) so the JSON rewrite is resilient when keys are
absent, then ConvertTo-Json and Set-Content as before.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 65c81052-a4ad-407c-ad1f-2a9c04ca1f6e
📒 Files selected for processing (1)
.github/workflows/sonarcloud.yml
Splits the Diffusion / Generated / NeuralNetworks ModelFamily shards by model
class (A-L / M-Z, Diffusion into 6) so each shard instantiates a bounded subset
instead of all paper-scale models at once — fixes the 16 GB ubuntu-latest OOM
("received a shutdown signal", 0 failures). Also adds matrix coverage for ~780
previously never-run tests (Document AI, Finance, integration suite).
Rebased cleanly onto master; net change is sonarcloud.yml only.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
0a2443d to
0f47197
Compare
1 participant
Summary
Fixes two problems in the test CI matrix (
sonarcloud.yml):1. OOM cancellations on the heavy ModelFamily shards
The 16 GB
ubuntu-latestrunner is killed ("received a shutdown signal" 2–6 min in, 0 test failures) on the Diffusion / Generated / NeuralNetworks shards. Each instantiates dozens of paper-scale models (~2.6 GB each: 880 MB weights + 1.76 GB Adam state). The existing mitigations — per-testGC.Collect(already inNeuralNetworkModelTestBase.DisposeAsync) and serialize+Workstation-GC for heavy shards — weren't enough because the cumulative distinct-model footprint per shard was too large.Fix: split finer so each shard loads fewer model classes, and serialize them:
A-C / D-I / J-M / N-R / S / T-Z;Salone = 49 models, the heaviest letter)A-L / M-Z, 85 models)A-M / N-Z, ~1670 methods)2. Entire test trees ran in ZERO shards (the "missing models" you noticed)
No shard matched these, so they never executed in CI:
AiDotNet.Tests.IntegrationTests(~686 files) — incl. Document AI (IntegrationTests/Document/*: LayoutAware, PixelToSequence, VisionLanguage, GraphBased), Finance, ComputerVision, Audio, Video, MetaLearning, Optimizers, Preprocessing, Statistics, … → added 10 gap-free letter-grouped Integration shards (A-B, C, D, E-G, H-L, M, N-O, P-Q, R, S, T-Z— partitions all of A–Z).FederatedLearning(50 files),Audio,Onnx,Tokenization,ComponentTests,AdversarialRobustness,EndToEndTests→ 3 shards.Model-instantiating Integration/TopLevel groups are serialized via
$heavyShards; light ones run parallel.Heads-up
The newly-covered shards will likely surface pre-existing failures in model families that were never run in CI — that's the intent (it's why Document AI / Financial AI looked "missing"). Those become tracked follow-ups; this PR establishes the coverage and stops the OOM cancellations.
Verification
ci-shard-closure-policy.ymlis an issue-close guard and needs no change.🤖 Generated with Claude Code
Summary by CodeRabbit