Skip to content

fix(profiler): gracefully handle stopping unstarted actions in Advanc… - #22005

Open
dai123x wants to merge 1 commit into
Lightning-AI:masterfrom
dai123x:fix/advanced-profiler-unstarted-action
Open

dai123x wants to merge 1 commit into
Lightning-AI:masterfrom
dai123x:fix/advanced-profiler-unstarted-action

Conversation

@dai123x

@dai123x dai123x commented Oct 4, 2026 •

Copy link
Copy Markdown

What does this PR do?

Fixes #9136.

AdvancedProfiler.teardown() cleared profiled_actions unconditionally. If a profiling context was still active when teardown ran, its action had already been dropped from the dict, so the context's finally block then called stop() on an action that no longer existed:

ValueError: Attempting to stop recording an action (run_test_evaluation) which was never started.

That is reachable in normal use: the profiler is torn down at the end of a stage while a profile(...) block around the test evaluation can still be unwinding.

Changes

  • teardown() disables any still-running profiles before clearing the dict, so nothing is left enabled once the profiler goes away.
  • stop() logs at debug level and returns instead of raising when the action was never started. This path is reachable from teardown() by construction, so the exception was never actionable for the caller.
  • Removes the now-inaccurate Raises: entry from the AdvancedProfiler docstring.
  • CHANGELOG entry added under the unreleased fixes.

Tests

Two regression tests in tests/tests_pytorch/profilers/test_profiler.py:

  • test_advanced_profiler_stop_unstarted_action - stop() on an action that was never started returns without raising.
  • test_advanced_profiler_teardown_disables_active_action - an action still active at teardown() is disabled and cleared.

What I ran and what I did not: I did not run the suite locally, because this workspace has no torch environment installed. Those two tests plus the existing profiler tests are what CI needs to run; if either fails I will fix it on this branch.

No breaking changes: the only behaviour removed is an exception that could not be recovered from and always followed a teardown.

@codecov-commenter

codecov-commenter commented Oct 5, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79%. Comparing base (84df182) to head (1557458).
✅ All tests successful. No failed tests found.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

❗ There is a different number of reports uploaded between BASE (84df182) and HEAD (1557458). Click for more details.

HEAD has 6067 uploads less than BASE
Flag BASE (84df182) HEAD (1557458)
cpu 1403 49
lightning_fabric 620 0
pytest 703 0
python3.12 583 21
python3.12.7 83 3
python3.13 570 19
python 83 3
lightning 166 5
python3.10 84 3
pytorch_lightning 617 44
pytorch2.6 124 9
pytest-full 700 49
pytorch2.12 83 5
pytorch2.7 42 3
pytorch2.9 81 6
pytorch2.8 81 6
pytorch2.13 123 8
pytorch2.10 83 6
pytorch2.11 83 6
Additional details and impacted files
@@            Coverage Diff            @@
##           master   #22005     +/-   ##
=========================================
- Coverage      87%      79%     -8%     
=========================================
  Files         270      267      -3     
  Lines       24015    23959     -56     
=========================================
- Hits        20855    18827   -2028     
- Misses       3160     5132   +1972     

@dai123x

dai123x commented Oct 5, 2026

Copy link
Copy Markdown
Author

The previous Test PyTorch run had two matrix jobs cancelled externally mid-run (no test failures — both were past 74% and passing), which failed pl-cpu-guardian and required-jobs. The docs linkcheck failures are pre-existing NVIDIA docs 404s, also failing on master. The newly pushed retrigger commit (ac08704) is held at action_required — could someone approve and re-run the workflows? Thanks!

@dai123x

dai123x commented Oct 9, 2026

Copy link
Copy Markdown
Author

Gentle nudge on this one. All six workflows on the current head (ac08704) are sitting at action_required, so nothing has actually run yet:

  • Test PyTorch
  • Code check
  • Docs builds
  • Package
  • Check Schema
  • Probot

Could someone approve the runs? For context on the earlier attempt: the previous Test PyTorch failure was two matrix jobs cancelled externally mid-run (both past 74%, no test failures), which took down pl-cpu-guardian and required-jobs; the docs linkcheck 404s are pre-existing NVIDIA URLs that also fail on master. The change itself is covered by tests and Codecov reports all modified lines covered.

Thanks!

…edProfiler (Lightning-AI#9136)

`AdvancedProfiler.teardown()` cleared `profiled_actions` unconditionally, so a
profiling context that was still active at teardown had its action dropped from
the dict. When that context exited, its `finally` block called `stop()` on an
action that no longer existed and raised:

    ValueError: Attempting to stop recording an action (run_test_evaluation) which was never started.

Changes:

- `teardown()` now disables any still-running profiles before clearing the dict,
  so no profile is left enabled after the profiler goes away.
- `stop()` logs at debug level and returns instead of raising when the action was
  never started. This path is reachable from `teardown()` by construction, so
  raising there was never actionable for the user.
- Removes the now-inaccurate `Raises:` section from the `AdvancedProfiler`
  docstring.
- Adds a regression test for stopping an action that was never started, and for
  teardown while an action is still active.

Fixes Lightning-AI#9136
@dai123x
dai123x force-pushed the fix/advanced-profiler-unstarted-action branch from ac08704 to 50324fc Compare October 23, 2026 09:07

This branch has not been deployed

No deployments
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.

AdvancedProfiler: ValueError: Attempting to stop recording an action (run_test_evaluation) which was never started.

2 participants