Add nvbench-compare-robust --plot-output/--plot-along-output options - #433
Conversation
The intent is to allow producing plotting artifacts in headless environments. Closes NVIDIA#395
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe PR adds explicit file outputs for summary and ChangesPlot output handling
Assessment against linked issues
Suggested reviewers: Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…related data Introduced _nvbench_compare_plotting.py helper that contains class PlotCollector. PlotCollector owns plotting-related state and rendering, and compare_benches() interacts with plotting through that collector. compare_benches() still produces comparison facts, but delegates plotting state management and rendering to PlotCollector.
06becf1 to
e37d351
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
| - `{benchmark}`: benchmark name | ||
| - `{device}`: compare-device id | ||
| - `{axis}`: selected plot axis name | ||
| - `{pair}`: zero-based positional device-pair index |
There was a problem hiding this comment.
{pair} is the zero-based index of (reference_device_id, compare_device_id) pairs that specification of --reference-devices/--compare-devices implies as specified. For example, specifying --reference-devices 0,1 --compare-devices 0,0, the pair (0, 0) would have index 0, and the pair (1, 0) would have index 1.
|
One issue found is that use of |
Because choice of matplotlib backend is global and must be perform before matplotlib is imported, it not technically possible to display one plot using one backend and generate figure using another. Use of Agg backend requires that all plots were generated as files. Added validation that either no file plot output is requested or it is requested for all plots.
On filename collision for plotting artifacts duplicates are resolved by appending `-copy-N` suffix to the filename. This applies to both collissions among artifacts generated by the script and to collussions with pre-existing files on disk. Echo plotting action to stderr rather than to stdout to avoid contamination of stdout a user might want to save/reuse Catch ValuError possibly raised by plt.savefig Strengthen tests to detect expected output 1. gives each file pair a filename-derived benchmark name, so wrong cross-file pairing becomes observable 2. Added - assert output.parent.is_dir() to the existing plot-along and summary plot save tests. - A focused test_save_or_show_plot_creates_parent_directories case for nested output parents.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
python/scripts/_nvbench_compare_plotting.py (3)
98-129: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuesuggestion:
resolve_plot_output_pathmixesos.path.exists(output)withcandidate_path.exists()and re-stats fromcounter = 1on every call. For directory comparisons producing many collisions this is O(n²) stat calls and the two existence checks read inconsistently. Not a correctness problem at realistic scale, but a per-stem counter cache would tidy both.
132-186: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion:
format_plot_along_output_pathre-runsvalidate_plot_along_output_templatefor every figure even thoughmain()already validated the template once at line 3358 ofpython/scripts/nvbench_compare_robust.py. Cheap enough, but the double validation makes ownership of the check ambiguous; consider validating once inPlotCollector.__post_init__and dropping it from the per-figure path.
579-611: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: The output path is reserved (and the collision warning printed) before
plot_comparison_entriesruns, so a latersavefigfailure leaves a reserved-but-unwritten path inoutput_pathsand a warning about a file that was never created. Reserving after a successful save would keep the registry truthful; low impact since the failure path aborts the run.python/scripts/nvbench_compare_robust.py (1)
3350-3371: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion:
validate_plot_output_modesruns here and again insidePlotCollector.__post_init__for every JSON pair. Keeping it in the collector is right for direct API callers, so themain()call is only for early exit before I/O — worth a one-line comment saying so, otherwise it reads as accidental duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 891c50e2-073a-4582-81a1-ee9b32dd5a5b
📒 Files selected for processing (5)
docs/nvbench_compare_robust.mdpython/scripts/_nvbench_compare_plotting.pypython/scripts/nvbench_compare_robust.pypython/test/test_nvbench_compare_robust.pypython/test/test_nvbench_tooling_deps.py
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
python/scripts/_nvbench_compare_plotting.py (3)
104-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion:
resolve_plot_output_pathtreats any pre-existing file as unavailable, so re-running the same command with a fixed--plot-output out.pngnever overwrites and instead accumulatesout-copy-1.png,out-copy-2.png, … Intra-invocation collision avoidance is clearly right; silently refusing to replace stale output from a previous run is a separate policy decision. Consider limiting the on-diskexists()check to plot-along template expansion, or documenting the never-overwrite guarantee explicitly.Also confirm
docs/nvbench_compare_robust.mddescribes the rename-and-warn behavior rather than the "raise an error" wording in the PR description.
386-386: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion:
plot_alongemptiness is tested inconsistently across the class — truthy at Line 386 and Line 463,is Noneat Line 435 and Line 494. With--plot-along ""this diverges:__post_init__skips the pyplot/seaborn setup,record_plot_alongstill passes itsis Noneguard and fails insideextract_plot_axis_valuewith--plot-along axis '' is not present in benchmark ..., whilerender_plot_alongsilently no-ops via theself.plt is Noneguard. Pick one predicate (or normalize""toNonein__post_init__) so an empty axis name is rejected once, up front.Normalize in __post_init__
def __post_init__(self) -> None: + if self.plot_along is not None and not self.plot_along: + raise ValueError("--plot-along requires a non-empty axis name") self.force_noninteractive_backend = validate_plot_output_modes(
444-460: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion:
axis_namehere holds the series label fromformat_plot_series_key(state name + occurrence + non-plotted axis values), not an axis name — andself.plot_alongin the same method is the axis name. Rename toseries_keyto matchformat_plot_series_key.python/scripts/nvbench_compare_robust.py (1)
3148-3159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuesuggestion: the new inner
cmp_deviceat Line 3150 reuses the name that Line 3164 assigns for the device-pair scope, and it leaks past the state loop. It's correct today only because Line 3164 reassigns before any use — rename tocmp_state_deviceto keep that safe under future edits.Rename
- cmp_device = find_device_by_id( + cmp_state_device = find_device_by_id( cmp_state["device"], run_data.cmp_devices ) plot_collector.record_summary_entry( benchmark_name=cmp_bench["name"], axis_label=axis_label, - cmp_device_name=cmp_device["name"] if cmp_device else None, + cmp_device_name=( + cmp_state_device["name"] if cmp_state_device else None + ),
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 306295a9-4dc4-42f6-87b2-d91833e876f2
📒 Files selected for processing (5)
docs/nvbench_compare_robust.mdpython/scripts/_nvbench_compare_plotting.pypython/scripts/nvbench_compare_robust.pypython/test/test_nvbench_compare_robust.pypython/test/test_nvbench_tooling_deps.py
| ), | ||
| tool_name=self.tool_name, | ||
| ) | ||
| use_noninteractive_matplotlib_backend(matplotlib) |
There was a problem hiding this comment.
critical: This switches the whole process to Agg, but plot_comparison_entries only forces Agg when it has its own output (line 175). So --plot --plot-along A --plot-along-output out.png (summary interactive, plot-along to file) saves the plot-along figure and then calls plt.show() for the summary under a forced Agg backend, but matplotlib emits FigureCanvasAgg is non-interactive, and thus cannot be shown and the summary figure is silently lost.
There was a problem hiding this comment.
The option validation is performed in nvbench_compare_robust.py that raises in case like this, where one plotting output is interactive and another is to file.
| def is_positive_finite(value): | ||
| return value is not None and math.isfinite(value) and value > 0.0 | ||
|
|
||
|
|
||
| def is_usable_noise(value): | ||
| return value is not None and math.isfinite(value) and value >= 0.0 |
There was a problem hiding this comment.
Important: these are redefined in nvbench_compare_robust.py
There was a problem hiding this comment.
I am going to leave it as is. Since nvbench_compare_robust.py imports _nvbench_compare_plotting deduplication means that nvbench_compare_robust would need to reuse definitions in the plotting module.
This kind of violates separation of concerns. I wanted _nvbench_compare_plotting to be focused on logic related to plotting.
These reused functions are tiny.
| if normalized_output_path not in output_paths and not os.path.exists(output): | ||
| output_paths.add(normalized_output_path) | ||
| return output |
There was a problem hiding this comment.
Question: is it intentional to not overwrite files already on disk from previous invocations? Wouldn't those files be considered stale?
There was a problem hiding this comment.
Yes, that was intentional to make patch small. The issue was that a duplicate may be discovered after a plotting artifact has already been created. Raising an error required a clean-up which required tracking which files were already created.
Avoiding this situation and still raising about duplicates required a dry run first to collect filenames to be written out, followed by validation and execution.
A quick and dirty fix was to always create a new file and disambiguate filename using a prefix.
There was a problem hiding this comment.
That makes sense for collisions within a run, but my question was about the os.path.exists() term. If we remove it, could we still avoid the duplicates within a single invocation but overwrite stale outputs from previous invocations?
There was a problem hiding this comment.
Yes, removing the os.path.exists() term would still avoid duplicates within a single nvbench-compare-robust invocation. The command owns one output_paths set in main() and passes it through all compare_benches() calls, so repeated resolved paths from the same process are still detected. The os.path.exists() term only protects pre-existing files from previous invocations.
For example,
nvbench-compare-robust \
--plot-along Elements \
--plot-along-output "plots/{benchmark}-{axis}.png" \
run1/ref.json run1/cmp.json
nvbench-compare-robust \
--plot-along Elements \
--plot-along-output "plots/{benchmark}-{axis}.png" \
run2/ref.json run2/cmp.json
If both files contain benchmark copy and axis Elements, both commands resolve to plots/copy-Elements.png output filename, with running command for run2/ overwriting output of command for run1/.
If we want normal CLI overwrite semantics for stale outputs, it can be removed without reintroducing same-invocation overwrites.
2 participants
The intent is to allow producing plotting artifacts in headless environments without relying on Linux-centric
"DISPLAY"environment variable and allowing user to select matplotlib's backend via environment variables.--plot-output PATHfor summary plots (i.e.,--plotwas specified).--plot-along-output PATH_OR_TEMPLATEfor--plot-along.plt.show().fig.savefig(..., dpi=150)and skipplt.show().--plot-along-outputsupports{benchmark},{device},{axis},{pair}templates.In addition, plotting data and their manipulation have been decoupled from
compare_benchesare now performed usingPlotCollectorclass. All plotting-related functionality has been move to helper source file_nvbench_compare_plotting.pyresiding next tonvbench_compare_robust.py.Closes #395