Skip to content

Address PyHPC profiler notebook review#242

Merged
brycelelbach merged 5 commits into
NVIDIA:event/2026-07-cscs-summer-schoolfrom
robobryce:fix/pyhpc-notebook-review
Jul 26, 2026
Merged

Address PyHPC profiler notebook review#242
brycelelbach merged 5 commits into
NVIDIA:event/2026-07-cscs-summer-schoolfrom
robobryce:fix/pyhpc-notebook-review

Conversation

@robobryce

@robobryce robobryce commented Jul 26, 2026

Copy link
Copy Markdown

Summary

Addresses all notebook feedback from #215 (comment) in a separate PR targeting event/2026-07-cscs-summer-school.

Notebook 03

  • Use the requested developer-tools setup wording.
  • Split the baseline correctness check into its own cell with the requested type assertion, numerical check, and output.
  • Add the same requested validation structure to the async implementation.
  • Explain what %%nsys does instead of exposing kernel-selection implementation details.
  • Replace the inaccurate timeline/extra-credit text with the three supported ways to inspect reports: Perfetto, the JupyterLab Nsight Systems app, or a local GUI.
  • Remove the report-path placeholder cells.

Notebook 04

  • Use the requested setup wording and remove the Colab/custom-kernel caveat.
  • Remove launch_blocked and launch_optimized, inlining launches at correctness, profiling, and benchmarking sites.
  • Reset dst before correctness checks and profiling, while leaving benchmark launches reset-free.
  • Restore Nsight Compute regex kernel filters for both kernels.
  • Expand the report guidance to cover the richer JupyterLab/local Nsight Compute GUI.
  • Add a guided code cell to the further-exploration section.
  • Normalize code-cell whitespace.

Notebook 05

  • Use eight items per thread for every histogram variant.
  • Apply the requested setup-cell spacing.
  • Remove launch_global and inline the histogram reset outside benchmarks.
  • Remove explicit device synchronization calls.
  • Combine launch/check/plot cells for both global and localized histograms.
  • Simplify the intentionally-racy assertion as requested.
  • Restore Nsight Compute regex kernel filters.
  • Remove the incorrect “Cooperative Groups” terminology.
  • Normalize code-cell whitespace.

The exercise and solution notebooks receive matching structural updates. Saved execution outputs and execution counts are cleared from the touched notebooks so the edited sources do not retain stale profiler results.

Testing

  • python3 brev/test-notebook-format.py <six changed notebooks>
  • pre-commit run notebook-format --files <six changed notebooks>
  • Parsed every non-magic code cell with Python ast.parse
  • Audited all six notebooks for empty saved outputs, null execution counts, no code-cell trailing newlines, and no repeated blank lines
  • git diff --check
  • Pre-push hooks: notebook format, Git LFS, and signed-commit verification all passed

GPU notebook execution was not available in this worktree: nvidia-smi cannot communicate with a driver, and the host Python environment does not include pytest. A full pre-commit run --all-files also reaches unrelated pre-existing canonical-format failures in events/pycon_2025/ai_with_a_conscience.ipynb and docs/notebook_template.ipynb; all six files changed here pass the same hook.

Live CSCS validation

Validated exact PR head c2c42e3018f8f4381cc74a2696a9f12d22345dd6 in Daint job 4276823 on nid005771. The fallback used the published event base image at 79d2b51, applied the exact Dockerfile nsightful-ncu install command from this PR, and executed the exact PR notebook 04/05 solutions.

Generated and imported four fresh reports: copy_blocked.ncu-rep (137,263,595 bytes), copy_optimized.ncu-rep (23,602,371 bytes), histogram_global.ncu-rep (66,766,950 bytes), and histogram_localized.ncu-rep (2,085,808 bytes). Each import contained the expected kernel and Duration metrics, with no Nsightful UsageError or skipped/no-kernel-captured warnings.

@copy-pr-bot

copy-pr-bot Bot commented Jul 26, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions

Copy link
Copy Markdown
Contributor

👋 Thank you for your contribution!

This pull request is from a forked repository so GitHub Actions will not be able to run CI. A maintainer will review your changes shortly and manually trigger the CI.

@maintainers Please review this PR when you have a chance and follow the instructions in the CONTRIBUTING.md file to trigger the CI.

@brycelelbach

Copy link
Copy Markdown
Collaborator

You have to test on CSCS. Not locally. Unacceptable.

@robobryce

Copy link
Copy Markdown
Author

Live CSCS testing found that the pinned Nsightful revision rejects NCU collection options in %%ncu. Commit 3c20ec2 moves the combined kernel-name regex into the nsightful-ncu kernelspec installation (--profiler-args) and restores the four %%ncu cells to report-only arguments. The pinned wrapper was checked directly and a temporary kernelspec was generated to verify the resulting argv. Requesting a CSCS rerun of notebooks 04 and 05; this is not considered complete until fresh non-empty NCU reports are confirmed.

@robobryce

Copy link
Copy Markdown
Author

Live CSCS validation passed for exact PR head c2c42e3018f8f4381cc74a2696a9f12d22345dd6.

Validation environment and fallback method:

  • Daint job 4276823 on nid005771.
  • Started from the published event base image at 79d2b51.
  • Applied the exact nsightful-ncu install command from this PR's Dockerfile so the NCU kernelspec contained the combined wrapper-level kernel regex.
  • Executed the exact notebook 04 and 05 solution files from this PR head.

Fresh reports generated:

  • copy_blocked.ncu-rep: 137,263,595 bytes
  • copy_optimized.ncu-rep: 23,602,371 bytes
  • histogram_global.ncu-rep: 66,766,950 bytes
  • histogram_localized.ncu-rep: 2,085,808 bytes

ncu --import verified that each report contains its expected kernel and Duration metrics. The notebook run produced no Nsightful UsageError and no skipped/no-kernel-captured warnings.

This confirms the compatibility fix: collection filtering is configured on the pinned Nsightful NCU wrapper/kernelspec, while %%ncu keeps only report/export arguments.

@brycelelbach

brycelelbach commented Jul 26, 2026 via email

Copy link
Copy Markdown
Collaborator

@robobryce

Copy link
Copy Markdown
Author

Addressed in e3e7a2d.

The notebook cells again pass their own specific regexes, for example:

%%ncu -o copy_blocked.ncu-rep --kernel-name regex:copy_blocked

I fixed Nsightful rather than moving those filters into the kernelspec. The Nsightful change is brycelelbach/nsightful#5: %%ncu now accepts --kernel-name / -k, applies it while exporting that cell's NVTX range from the aggregate report, and keeps display arguments separate.

ACH now pins that exact signed Nsightful commit and the kernelspec is back to generic --set full --clock-control none configuration. Nsightful's full CPU test suite passes (245 passed, 2 skipped), its pre-commit hooks pass, and all four modified ACH notebooks pass canonical-format checks.

I am continuing with live CSCS validation of the notebook-level filters before considering this resolved.

@brycelelbach
brycelelbach merged commit 95668e4 into NVIDIA:event/2026-07-cscs-summer-school Jul 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants