[Reporting] SOL for NCCL/NIXL in comparison report - #991
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughCloudAI adds typed SOL target configuration, precedence handling, NCCL and NIXL metric extraction, shared SOL comparison reporting, responsive chart updates, documentation, and integration tests. ChangesSOL metric reporting
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
podkidyshev
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/cloudai/metrics.py`:
- Around line 210-217: Update the attainment property to handle a zero
self.observation.value before the MINIMIZE division, returning an appropriate
non-error result consistent with the metric’s attainment semantics. Preserve the
existing target-none and MAXIMIZE behavior, and ensure zero-valued observations
cannot raise ZeroDivisionError during report generation.
In `@src/cloudai/report_generator/comparison_report_metric.py`:
- Around line 117-127: Update the result-column assignments in the comparison
report metric generation flow to use the shared SOL and attainment column-name
helpers from ComparisonReport instead of interpolating “ SOL” and “ % SOL”
directly. Reuse the same helpers for both writing columns and preserve the
existing mapping behavior.
- Around line 121-124: Update NIXLBenchComparisonReport’s comparison-series
construction to preserve batch_size, either by including batch_size in
series_dimensions or by explicitly aggregating rows so distinct batch sizes are
not collapsed by the groupby(...).first() call in the series filtering flow.
In `@src/cloudai/report_generator/comparison_report.py`:
- Around line 729-733: Update the measured_max and measured_min reductions in
_build_curve_section to use NaN-aware pandas/NumPy aggregation, then exclude any
NaN results before computing y_max and y_min. Preserve the existing sol_values
inclusion and ensure Range1d receives finite measured bounds when data columns
are entirely NaN.
- Around line 465-482: Update the SOL data construction around _shared_sol_curve
to use _numeric_value for x values and skip non-numeric points consistently with
the measured branch. Replace the datasets.insert call with datasets.append, then
remove the now-unused metric_idx and inserted counter by dropping enumerate and
related increments.
In `@src/cloudai/workloads/nccl_test/nccl.py`:
- Around line 197-228: Update metric_observations to validate all required
numeric fields in each row before converting them or creating MetricObservation
objects. Skip rows containing non-numeric values such as N/A, while preserving
existing handling for valid values and allowing -nan to follow current float
conversion behavior.
In `@src/cloudai/workloads/nixl_bench/nixl_bench.py`:
- Around line 70-77: Update the dimensions construction to access the typed
NIXLBenchCmdArgs fields directly for op_type, backend, initiator_seg_type, and
target_seg_type instead of using getattr defaults. Preserve the lowercase string
conversion, and handle any optional None values explicitly rather than
converting them to "none"; keep size_bytes and batch_size unchanged.
In `@tests/workloads/nixl_bench/test_report.py`:
- Around line 97-105: Extend the report tests around the existing legacy_html
and html assertions to cover two compared runs with different SOL values,
exercising the _shared_sol_curve None path, and verify the latency section omits
SOL and % SOL columns when no SOL target exists. Replace the broad legacy_html
SOL substring check with an assertion for the rendered SOL column header.
🪄 Autofix
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: Enterprise
Run ID: 5637b817-d2e1-4bf9-8627-b947a375bfc3
📒 Files selected for processing (15)
doc/reporting.rstsrc/cloudai/_core/system.pysrc/cloudai/_core/test_scenario.pysrc/cloudai/metrics.pysrc/cloudai/models/scenario.pysrc/cloudai/models/workload.pysrc/cloudai/report_generator/comparison_report.pysrc/cloudai/report_generator/comparison_report_metric.pysrc/cloudai/test_scenario_parser.pysrc/cloudai/util/comparison-report-v2.jinja2src/cloudai/workloads/nccl_test/nccl.pysrc/cloudai/workloads/nccl_test/nccl_comparison_report.pysrc/cloudai/workloads/nixl_bench/nixl_bench.pysrc/cloudai/workloads/nixl_bench/nixl_summary_report.pytests/workloads/nixl_bench/test_report.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/cloudai/workloads/nccl_test/nccl.py (1)
204-211: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReject non-finite benchmark values before creating observations.
float("nan")and infinity pass numeric conversion, butMetricObservationrejects non-finite values. One invalid result row then aborts report generation instead of being skipped.
src/cloudai/workloads/nccl_test/nccl.py#L204-L211: Check converted latency and bandwidth values withmath.isfinitebefore appending observations.src/cloudai/workloads/nixl_bench/nixl_bench.py#L71-L88: Catch conversion failures and skip rows with non-finite latency or bandwidth before constructing observations.🤖 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 `@src/cloudai/workloads/nccl_test/nccl.py` around lines 204 - 211, In src/cloudai/workloads/nccl_test/nccl.py lines 204-211, update the row parsing in the NCCL observation flow to skip rows when any converted latency or bandwidth value is non-finite, using math.isfinite before appending observations. In src/cloudai/workloads/nixl_bench/nixl_bench.py lines 71-88, catch numeric conversion failures and skip rows whose converted latency or bandwidth values are non-finite before constructing MetricObservation instances.
🤖 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/cloudai/report_generator/comparison_report.py`:
- Around line 384-395: The _sol_curve method currently collapses conflicting SOL
targets with drop_duplicates(x_column), producing an arbitrary shared curve.
Update it to render separate SOL datasets, or return no shared curve unless all
valid SOL values agree for each x value; preserve the existing curve only when
candidates are consistent, and add coverage for differing batch-size or
placement SOL targets.
---
Duplicate comments:
In `@src/cloudai/workloads/nccl_test/nccl.py`:
- Around line 204-211: In src/cloudai/workloads/nccl_test/nccl.py lines 204-211,
update the row parsing in the NCCL observation flow to skip rows when any
converted latency or bandwidth value is non-finite, using math.isfinite before
appending observations. In src/cloudai/workloads/nixl_bench/nixl_bench.py lines
71-88, catch numeric conversion failures and skip rows whose converted latency
or bandwidth values are non-finite before constructing MetricObservation
instances.
🪄 Autofix
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: Enterprise
Run ID: 330d0ce9-7ad8-4fda-bd82-a05c76a37ea5
📒 Files selected for processing (7)
src/cloudai/metrics.pysrc/cloudai/report_generator/comparison_report.pysrc/cloudai/report_generator/comparison_report_metric.pysrc/cloudai/workloads/nccl_test/nccl.pysrc/cloudai/workloads/nccl_test/nccl_comparison_report.pysrc/cloudai/workloads/nixl_bench/nixl_bench.pytests/workloads/nixl_bench/test_report.py
💤 Files with no reviewable changes (1)
- src/cloudai/workloads/nccl_test/nccl_comparison_report.py
podkidyshev
marked this pull request as ready for review
August 7, 2026 14:41
podkidyshev
requested review from
jj10306,
rutayan-nv and
srivatsankrishnan
as code owners
August 7, 2026 14:42
jj10306
requested changes
Aug 10, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/cloudai/metrics.py`:
- Around line 86-93: Update the class methods register_metrics and
register_dimensions to explicitly annotate their return type as None, preserving
their existing registry mutation behavior and satisfying Ruff ANN206.
In `@tests/test_metrics.py`:
- Around line 61-75: Add regression coverage in the metrics tests alongside
test_merge_sol_configs for assess_observation or assess_test_run_metrics using a
zero-valued MINIMIZE observation. Assert that the assessment returns the
expected unavailable attainment result and avoids division-by-zero during report
generation.
🪄 Autofix
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: Enterprise
Run ID: fcaa672f-4d37-4d63-b88e-d3dd7f0f6c26
📒 Files selected for processing (2)
src/cloudai/metrics.pytests/test_metrics.py
jj10306
approved these changes
Aug 10, 2026
jj10306
left a comment
jj10306
left a comment
There was a problem hiding this comment.
lgtm! please clarify https://github.com/NVIDIA/cloudai/pull/991/changes/BASE..013ebc6d9698875d669b19becbfd7976d414186a#r3753277571 before merging
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
Flexible SOL configuration + using it in comparison reports for NCCL and NIXL
Test Plan
Additional Notes
Trade-off
The complexity of comparison reporters has grown significantly. I attempted many solutions that both produce clear code and stay under the sane limit of code changes. Unfortunately, we need a complete rebuild of data structures.
I'll create an internal ticket for this once we pass this PR. The PR change is a P1 requirement for another team thus it cannot take forever to iterate on it...
Example: NIXL
Example: NCCL