Repository navigation
ci(nightly): open the Slack thread at nightly start, like dynamo - #353
Conversation
- slack-thread-start posts dynamo's root message as soon as the run is cleared to reach the GitLab trigger, instead of after the build. - slack-staging-reply posts "Staging complete" with the wheel and crate versions. - trigger-gitlab-security replies "GitLab release pipeline triggered" and forwards the root ts, so GitLab replies land in the same thread. - slack-thread-stopped closes the thread when a later gate stops the run. Signed-off-by: pvijayakrish <pvijayakrish@nvidia.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: ai-dynamo/aisimulate/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (22)
🧰 Additional context used📓 Path-based instructions (5)Only root workflows are active.⚙️ CodeRabbit configuration file Files:
Require coverage of the changed behavior and its negative or boundary cases.⚙️ CodeRabbit configuration file Files:
Read REVIEW.md before commenting.⚙️ CodeRabbit configuration file Files:
Before making any change under: `python/aisimulate/src/aisimulate/generator/**` MUST read: `python/aisimulate/.claude/rules/generator-development.md` Before making any change under `python/aisimulate/collector/**` MUST read: `python/aisimul...📄 CodeRabbit inference engine (AGENTS.md) Files:
Source excerpt: Only workflows under the repository-root `.github/workflows/` run for this repository.📄 CodeRabbit inference engine (REVIEW.md) Files:
🪛 ast-grep (0.45.3)tests/test_ci_workflow_contracts.py[error] 2829-2850: Command coming from incoming request (subprocess-from-request) 🔀 Multi-repo context ai-dynamo/dynamo, ai-dynamo/aiconfiguratorLinked repositories findingsai-dynamo/dynamo
ai-dynamo/aiconfigurator
🔇 Additional comments (2)
📝 SummaryRisk: Medium. The three areas needing the most human attention are workflow job conditions and dependencies, Slack credentials and best-effort posting, and GitLab trigger response handling.
WalkthroughThe nightly workflow now posts Slack thread updates for eligible runs. It reports staging artifact results and GitLab pipeline details, or posts a stopped notice when the GitLab trigger does not succeed. ChangesNightly Slack notifications
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The nightly notifications preserve release triggering when Slack posting fails and close existing threads when later gates stop the run. No merge-blocking issue remains; live Slack and GitLab posting has not been validated. 🚥 Pre-merge checks | ✅ 4 | ❌ 3 | ❓ 1❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description states the intended Slack-thread behavior, but the required template fields remain empty. Review risk, starting points, contract impact, compatibility concerns, test results, reviewed commits, boundary cases, and tracking information are missing. Resolution Complete the template. Add the problem and intended consumer under Why and what changed, fill the Review map, provide exact validation commands and results under Evidence, write N/A or provenance details under Modeling or data provenance, and complete Closes and Related PRs or issues. Full details: Cross-Layer ContractExplanation The PR adds new cross-layer CI contracts without complete consumer tests or documentation. Resolution Add executable workflow-contract tests with a fake curl/Slack API. Cover root-message success and missing Full details: Modeling And Data EvidenceExplanation The PR changes nightly selection and gating logic. It adds the Resolution Add reproducible evidence for the changed nightly selection behavior. Include the source or provenance for the expected gate rules, plus an explained before/after comparison or independent oracle. A machine-readable scenario matrix should cover schedule, manual approval, retry attempt, branch, build guard, trigger flag, skipped/failing dependencies, and Slack failure cases, with anomaly checks and recorded results. Full details: Review EvidenceExplanation The PR description does not provide commands, results, negative or boundary validation, or distinctions between local, fake-runner, parity, and production evidence. It also does not cite an inactive nested workflow as hosted CI evidence. The available description therefore does not establish the pass criteria or the stated fail condition. Resolution Update the PR description with exact validation commands and results, negative or boundary cases for the Slack and workflow gates, and clear labels for local, fake-runner, parity, and production evidence. State any reusable workflow's activation mode and hosted CI result explicitly. Comment |
Harrilee
left a comment
Harrilee
left a comment
There was a problem hiding this comment.
LGTM. The gating looks right: slack-thread-start uses the same approval/attempt/GITLAB_SECURITY_TRIGGER_ENABLED gate as trigger-gitlab-security. The trigger job keeps !cancelled() and never checks the Slack job's result, so a Slack outage still can't block the GitLab trigger. The follow-up jobs skip cleanly when thread-ts is empty, and the crate version derivation (X.Y.Z.devN → X.Y.Z-dev.N) matches the versioning contract in the header. The GitLab response now goes to a file and is never echoed, so the internal pipeline URL stays out of the public log. I ran the nightly/gitlab contract tests locally on d355254: 45 passed.
A few non-blocking nits:
- Slack post boilerplate is repeated 4 times. The same
curl chat.postMessage+.okcheck block appears inslack-thread-start,slack-staging-reply, the GitLab reply step, andslack-thread-stopped. Consider pulling it into a small script or composite action so later fixes (retry flags, error handling) only have to happen in one place. - Extra
automated-releasedeployments. Every new Slack job bindsenvironment: automated-releasejust to read the Slack secrets, so each nightly now creates about 4 extra deployment records for that environment. That's fine if intended. Otherwise, moving the two Slack secrets to repo level (likeSLACK_NOTIFY_NIGHTLY_WEBHOOK_URL) would avoid it. - Silent Slack failures. Every Slack step uses
continue-on-error: true, sonotify-slacknever flags a broken Slack thread; it only shows up as an::error::annotation. That's acceptable for best-effort, but worth knowing. slack-thread-stoppedwording. On a cancelled run, or when the GitLab trigger itself fails, it still says "Nightly stopped before the GitLab pipeline". Includingneeds.trigger-gitlab-security.resultin the text would make the thread easier to triage.- Internal GitLab URL in Slack. The trigger reply now posts the internal GitLab pipeline URL to Slack. Please confirm that
SLACK_RELEASE_CHANNEL_IDis an internal-only channel. - PR description. The template sections (risk level, Evidence, Tracking) are still empty. Please fill in at least the risk level and the test evidence.
Brings in the FPM decoupling / self-service onboarding stack (ai-dynamo#238, ai-dynamo#248, ai-dynamo#347), the output adapters (ai-dynamo#334) and the CI changes (ai-dynamo#349, ai-dynamo#351, ai-dynamo#353, ai-dynamo#354, ai-dynamo#330, ai-dynamo#319). Conflict resolutions: - ENGINE_SPEC_SCHEMA_VERSION: upstream claimed 25 for the FPM decoupling selector; decode CP is renumbered to 26 (positional dcp_size tails on the attention / MLA / DSA ops). Stale-payload loops reject 20..25; the 25 payload keeps the selector like the decoupling branch's 21. - cp_size: upstream added a CP1-only `cp_size` to compile_engine, estimate_kv_cache / estimate_num_gpu_blocks, EngineBuildRequest and the legacy Rust compile path ("this SDK entry point does not support context parallelism"). This branch supports prefill CP at exactly those entry points, so the duplicate parameters / struct field are folded into ours, the CP1 gates become positive-integer validation, and the FPM profile cell selection receives the real cp_size (a profile without that cell fails loud with "no matching FPM deployment profile"). Upstream's tests are adjusted accordingly. - FpmCompileConfig / AicTimingConfig parallel-shape checks combine upstream's `fpm_profile.is_none()` exemption with the `* cp_size` fold; aic_capacity_kwargs gains cp_size / dcp_size; fpm best_available keeps upstream's registered-architecture check ahead of the DCP mode gate. - capacity.py worker resolution carries aic_cp_size / aic_dcp_size next to aic_fpm_profile / worker_type. - ParallelismPresetConfig.prefill_context becomes Optional (None = 1), like decode_context, so default parallelism dumps carry no CP keys; the new onboarding topology tests (ai-dynamo#248) compare those dumps against the six-key request parallelism. Consumers read it through compiler._prefill_cp. Signed-off-by: Tianhao Xu <tianhaox@nvidia.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…dynamo#353) - slack-thread-start posts dynamo's root message as soon as the run is cleared to reach the GitLab trigger, instead of after the build. - slack-staging-reply posts "Staging complete" with the wheel and crate versions. - trigger-gitlab-security replies "GitLab release pipeline triggered" and forwards the root ts, so GitLab replies land in the same thread. - slack-thread-stopped closes the thread when a later gate stops the run. Signed-off-by: pvijayakrish <pvijayakrish@nvidia.com>
Why and what changed
Review map
Evidence
Modeling or data provenance
Tracking