feat(guardrails): Update benchmark job; add latency analysis step#359
Open
JashG wants to merge 9 commits into
Open
feat(guardrails): Update benchmark job; add latency analysis step#359JashG wants to merge 9 commits into
JashG wants to merge 9 commits into
Conversation
Contributor
|
JashG
force-pushed
the
jgulabrai/guardrails-benchmark-analysis
branch
from
6115c89 to
f78ffa8
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds guardrails benchmark change detection, dual benchmark variants, post-run analysis, and supporting benchmark configs, seeding, tests, workflow, and docs. ChangesGuardrails benchmark workflow
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yaml (1)
1306-1334: 🩺 Stability & Availability | 🟠 MajorAdd
guardrails-benchmarkandguardrails-benchmark-analyzetoci-statusdependencies.The ci-status job (lines 1306-1334) omits both benchmark jobs. If ci-status is the required gate and benchmark jobs fail (when triggered by changes or dispatch), those failures are missed.
Suggested patch
ci-status: name: CI status needs: - changes + - guardrails-benchmark + - guardrails-benchmark-analyze - actionlint - docker-bake-graph - build-cpu-smoke-images🤖 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/ci.yaml around lines 1306 - 1334, The ci-status job is missing guardrails-benchmark and guardrails-benchmark-analyze from its needs dependencies list. Add both of these job names to the needs array under the ci-status job (after the other existing job names like web-studio-e2e and opa-policy-test) to ensure benchmark job failures are reflected in the overall CI status gate.
🧹 Nitpick comments (1)
plugins/nemo-guardrails/benchmarks/configs/mock_llm/README.md (1)
1-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestructure this page to one Diataxis type with required sections.
This page is currently descriptive but missing required doc scaffolding. Convert it into a single quadrant (likely REFERENCE), add a top prerequisites block, add concrete CLI/Python examples in a tab-set, and end with a “Next Steps” section linking to related benchmark docs.
As per coding guidelines,Each documentation page should fit ONE Diataxis quadrant,Always list prerequisites at the top,Provide both Python SDK and CLI examples in tab-sets, andInclude 'Next Steps' section at the end.🤖 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 `@plugins/nemo-guardrails/benchmarks/configs/mock_llm/README.md` around lines 1 - 22, Restructure the Mock LLM configurations README to follow Diataxis REFERENCE format by adding a prerequisites section at the top that lists required dependencies and setup steps, reorganizing the existing descriptive content into a properly structured REFERENCE page, adding a new section with concrete CLI and Python SDK examples in tab-set format demonstrating how to use and configure the app-llm.env and content-safety-llm.env files, and ending with a "Next Steps" section that links to related benchmark documentation and guides for running benchmarks with these configurations.Source: Coding guidelines
🤖 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/ci.yaml:
- Around line 1142-1147: The analyze job's if condition prevents it from running
when the guardrails-benchmark job fails, even though the job handles missing
artifacts gracefully with continue-on-error. Modify the if condition that
currently checks for !cancelled() and the workflow_dispatch or changes.outputs
conditions to use always() instead, so the analyze job runs regardless of the
guardrails-benchmark job's status and can process partial artifacts when
available.
- Around line 1151-1154: The checkout action for nemo-platform is missing the
persist-credentials security setting. Add `persist-credentials: false` to the
`with:` section of the "Checkout nemo-platform" step to prevent the GitHub
Actions authentication token from being stored in the Git configuration, since
this job only needs to download and upload artifacts without performing Git
operations.
In `@plugins/nemo-guardrails/benchmarks/README.md`:
- Around line 227-229: The worked example in the benchmark README for c=16
tolerance incorrectly states the tolerance as 300 ms when the actual value used
in analyze.py is 200 ms. Update lines 227-229 to replace the 300 ms tolerance
value with the correct 200 ms value from analyze.py line 46, and recalculate the
pass/fail example thresholds accordingly so that the example accurately
demonstrates how the 200 ms tolerance determines whether a performance run
passes or fails based on the delta_p50 values.
In `@plugins/nemo-guardrails/src/nemo_guardrails_plugin/benchmarks/seeding.py`:
- Around line 181-194: The control VM creation in the seeding flow can reuse an
existing virtual model unchanged when create(..., exist_ok=True) returns it, so
the empty request_middleware and response_middleware settings may be ignored.
Update the no_guardrails_vm handling in the seeding logic by either calling
client.inference.virtual_models.update() after creation to explicitly clear
middleware, or deleting any existing NO_GUARDRAILS_VM_NAME before recreating it,
so the control VM always stays middleware-free.
---
Outside diff comments:
In @.github/workflows/ci.yaml:
- Around line 1306-1334: The ci-status job is missing guardrails-benchmark and
guardrails-benchmark-analyze from its needs dependencies list. Add both of these
job names to the needs array under the ci-status job (after the other existing
job names like web-studio-e2e and opa-policy-test) to ensure benchmark job
failures are reflected in the overall CI status gate.
---
Nitpick comments:
In `@plugins/nemo-guardrails/benchmarks/configs/mock_llm/README.md`:
- Around line 1-22: Restructure the Mock LLM configurations README to follow
Diataxis REFERENCE format by adding a prerequisites section at the top that
lists required dependencies and setup steps, reorganizing the existing
descriptive content into a properly structured REFERENCE page, adding a new
section with concrete CLI and Python SDK examples in tab-set format
demonstrating how to use and configure the app-llm.env and
content-safety-llm.env files, and ending with a "Next Steps" section that links
to related benchmark documentation and guides for running benchmarks with these
configurations.
🪄 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: CHILL
Plan: Enterprise
Run ID: 1cd5064a-464d-418f-a424-914014ecf570
📒 Files selected for processing (13)
.github/actions/changes/action.yaml.github/workflows/ci.yamlplugins/nemo-guardrails/benchmarks/README.mdplugins/nemo-guardrails/benchmarks/configs/mock_llm/README.mdplugins/nemo-guardrails/benchmarks/configs/mock_llm/app-llm.envplugins/nemo-guardrails/benchmarks/configs/mock_llm/content-safety-llm.envplugins/nemo-guardrails/src/nemo_guardrails_plugin/benchmarks/aiperf_runner.pyplugins/nemo-guardrails/src/nemo_guardrails_plugin/benchmarks/analyze.pyplugins/nemo-guardrails/src/nemo_guardrails_plugin/benchmarks/constants.pyplugins/nemo-guardrails/src/nemo_guardrails_plugin/benchmarks/paths.pyplugins/nemo-guardrails/src/nemo_guardrails_plugin/benchmarks/run.pyplugins/nemo-guardrails/src/nemo_guardrails_plugin/benchmarks/seeding.pyplugins/nemo-guardrails/tests/unit/benchmarks/test_seeding.py
JashG
force-pushed
the
jgulabrai/guardrails-benchmark-analysis
branch
from
ad12028 to
dcaf12f
Compare
JashG
changed the title
albcui
reviewed
Jun 24, 2026
Contributor
There was a problem hiding this comment.
Why not add guardrails-benchmark/guardrails-benchmark-analyze to this list to gate CI? Is the reasoning that we haven't established tight baselines yet?
Contributor
Author
There was a problem hiding this comment.
Since this only runs right now on changes to services/guardrails and plugins/nemo-guardrails, we should add it to the list. Updated.
| ``` | ||
|
|
||
| Why only delta_p50 (and not absolute with-guardrails p50)? delta_p50 | ||
| isolates the middleware's contribution — shared CI runner noise cancels |
Contributor
There was a problem hiding this comment.
shared CI runner noise cancels ...
This is not quite true because in CI, the two jobs could run on separate runners (machines). If runnerA happens to be slow and B fast, the delta is inflated (and vice versa). So the inter-runner noise is actually not cancelled.
| the numbers. | ||
|
|
||
| Worked example: at c=16 the override is 200 ms, so a run with observed | ||
| delta_p50 = 1689 (diff +199 from baseline 1390) passes; observed |
Contributor
There was a problem hiding this comment.
the math is off... 1390 + 199 = 1589
JashG
force-pushed
the
jgulabrai/guardrails-benchmark-analysis
branch
from
08025e7 to
b3c9b97
Compare
Signed-off-by: Jash Gulabrai <jgulabrai@nvidia.com>
Signed-off-by: Jash Gulabrai <jgulabrai@nvidia.com>
Signed-off-by: Jash Gulabrai <jgulabrai@nvidia.com>
Signed-off-by: Jash Gulabrai <jgulabrai@nvidia.com>
Signed-off-by: Jash Gulabrai <jgulabrai@nvidia.com>
Signed-off-by: Jash Gulabrai <jgulabrai@nvidia.com>
JashG
force-pushed
the
jgulabrai/guardrails-benchmark-analysis
branch
from
b3c9b97 to
db48258
Compare
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
2 participants
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.
Summary
guardrails-benchmark job
Updates the
guardrails-benchmarkCI step:nemo-guardrailsplugin.guardrails-benchmark-analyze job
Updates the
guardrails-benchmark-analyzeCI step:Measured Latencies: Total e2e latency for both benchmarks.Platform Overhead: Latency added by NMP (i.e. latencies without counting the mock LLM calls).Guardrails Overhead vs. Baseline: Latency added by Guardrails compared to pre-defined baselines. These baselines are the average of several runs in CI. I added a "reasonable +/- threshold" to account for potential latency deviations.Benchmark harness
run.pynow accepts--variant {with-guardrails, without-guardrails, all}.allruns both variants sequentially against the same NMP, writing them to the same run directory underaiperf_results/<variant>/.seeding.pycreates a second control VirtualModel (no-guardrails-vm) alongside the existing guarded one.aiperf_runner.pyaccepts amodel_refso each variant's sweep targets the correct VM.analyze.py(stdlib-only) reads both variants from a run dir and prints three tables: measured latencies, platform overhead (mock-LLM time subtracted), and the baseline check.Summary by CodeRabbit