Repository navigation
fix: give visualize.py a CLI so the documented command actually works - #56
Merged
Merged
Conversation
The module had no __main__ block and never imported argparse, so the python profiling/visualize.py command documented in four places exited 0 and wrote nothing. A silent no-op is worse than a crash, because the caller walks away believing the plots now exist. That is why the A100 result set had no plots or summary. Adds main() taking the results path positionally plus --run-dir, which defaults to the directory holding the JSON so the common case needs no second argument. A missing file is an argparse error rather than a traceback. os moves to module level since it is no longer local to one function.
The test job installed .[dev], but matplotlib lives in the profiling extra. Any test touching profiling.visualize would have failed on import in CI while passing locally. make setup already installs .[dev,profiling], so CI was the outlier.
The regression this guards against is a module with no __main__ block. Importing visualize would look perfectly healthy in that case, which is how the missing entry point survived: every documented invocation exited 0 having written nothing. So these tests run the script as a subprocess from the repo root, the way a contributor does. Covers --help rendering, the layout manifest.json describes (four plots under plots/, exactly one summary at run-dir level and none under plots/), --run-dir defaulting to the directory holding the JSON, and a missing input failing loudly rather than silently. testpaths widens to include profiling/tests, without which pytest would not collect any of this. The suite grows from 25 to 29 tests and from about 10s to about 62s locally, because two of them render four figures each. That cost is deliberate: a silent no-op is the failure mode being prevented.
The A100 result set held only benchmark_results.json and metadata.json while the T4 set had four plots and a summary. Nothing was missing from the measurement; the artifacts were simply never generated, because the command documented for doing it exited 0 and wrote nothing. Regenerated from the committed JSON with the new CLI. No GPU is involved and no measured value changes. The summary reports the A100 peaks from gpu_specs_db.py: 1555 GB/s, 19.5 TFLOP/s FP32, 312.0 TFLOP/s dense BF16 tensor. manifest.json now lists all seven A100 files, so both result sets declare the same artifact types. Verified both directions: every listed file exists and every file on disk is listed, for both runs. One figure worth stating because two documents quote different values. Average A100 attention compute utilization is 7.0 percent and the peak across the twelve attention configurations is 14.06 percent. The analysis document quotes the range and the peak; this summary quotes the mean. Recomputed from raw data: mean 6.9991, max 14.0571, min 0.9722. Neither number is wrong and neither should be reconciled to the other.
The A100_RESULTS path was wrapped across five lines; ruff collapses it to one because it fits inside the 100 character limit. Committed separately because the preceding commit added the file without running the formatter, which would have failed the lint job on this branch.
Four documents described a command that did not work. Three showed python profiling/visualize.py <results.json>, which exited 0 and wrote nothing, and two of those also passed --output-dir .../plots/, a flag that has never existed and a directory that is now the wrong target. CONTRIBUTING.md described the failure accurately but as an open defect; it has since landed, so it now documents the working command instead. All four show the real invocation and the layout it produces: four PNGs under plots/, one benchmark_summary.txt at run-directory level. Also corrects two things the stale docs hid. The profiling/README.md file structure tree still showed the flat pre-reorganization results layout and omitted gpu_specs_db.py, recompute_utilization.py and tests/. And the four charts in the root README did not say which GPU or run they came from, so a reader could not tell whether a figure described the T4 or the A100; each caption now names both, and the section says the A100 run has the same four charts in its own directory.
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
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.
Closes #48.
The bug
profiling/visualize.pyhad noif __name__ == "__main__"block and never imported argparse. Every documented invocation of it exited 0 and wrote nothing:A silent no-op is worse than a crash. Whoever ran it walked away believing the plots now existed. That is how the A100 result set came to have only
benchmark_results.jsonandmetadata.jsonwhile the T4 set had four plots and a summary, and it is why--output-dirappears in two documents despite never having been implemented.What changed
Six commits.
43eaa5amain()with argparse: positionalresults_path, optional--run-dir032d8bc.[dev,profiling], since matplotlib lives in the profiling extra7288a939b79674manifest.jsonupdatedc2d7332ruff formaton the new test file396918b--run-dirdefaults to the directory holding the JSON, so the common case needs no second argument. A missing input is an argparse error, not a traceback.The flag is
--run-dir, not the--output-dirthe old docs claimed. Naming it--output-dirwhile it means "result set directory" is what produced the duplicate summary in #47, so the name now says what it takes.The test executes the script rather than importing it
That distinction is the entire point. Importing
visualizewould have looked perfectly healthy while the bug was live, because the defect was the absence of an entry point. Only running it the way a contributor does catches that. The tests cover--helprendering, the layoutmanifest.jsondescribes,--run-dirdefaulting, and a missing file failing loudly.This required widening
testpathsto includeprofiling/tests, without which pytest collected none of it. That is a small piece of #35, which remains open for real profiling coverage.A100 artifacts
Generated from the committed JSON. No GPU involved, and no measured value changes anywhere. The summary reports the A100 peaks from
gpu_specs_db.py: 1555 GB/s, 19.5 TFLOP/s FP32, 312.0 TFLOP/s dense BF16 tensor.Both result sets now declare the same seven artifact types, verified in both directions: every file listed in
manifest.jsonexists, and every file on disk is listed.One figure stated explicitly, because two documents quote different numbers for it. Average A100 attention compute utilization is 7.0%; the peak across the twelve attention configurations is 14.06%. The analysis document quotes the range and the peak, this summary quotes the mean. Recomputed from raw data: mean 6.9991, max 14.0571, min 0.9722. Neither is wrong and neither should be reconciled to the other.
Documentation
Four files described the broken command.
profiling/README.mdandREADME.mdshowed the bare invocation,profiling/results/README.mdshowed it twice with the nonexistent--output-dir, andCONTRIBUTING.mddescribed the failure accurately but as an open defect. All four now show the working command and the layout it produces.Two further corrections the stale docs were hiding:
profiling/README.mdfile structure tree still showed the flat pre-reorganization results layout and omittedgpu_specs_db.py,recompute_utilization.py, andtests/.Verification
python -m ruff check .reportsAll checks passed!python -m ruff format --check .reports 25 files already formattedpython -m pytestpasses, 29 tests, up from 25--helprenders, and a real run against the A100 JSON produced four PNGs underplots/and onebenchmark_summary.txtat run-dir level, with nothing atplots/benchmark_summary.txtmanifest.jsonchecked against the filesystem for both result sets, both directions, zero mismatchesOne thing worth flagging about my own process. Commit
7288a93added the test file without running the formatter, so it failedruff format --checkand would have turned the lint job red on this branch.c2d7332fixes it. Caught locally before pushing, but it is a real miss and the reason that commit exists separately.Not included
The suite grows from about 10s to about 40-60s locally, because two of the new tests render four matplotlib figures each in a subprocess. That cost is deliberate: a silent no-op is the failure being prevented, and only real execution prevents it.
Full profiling test coverage is #35. This adds one test module, not a suite.