fix: attribute user-triggered warnings to the caller via stacklevel - #4992
Closed
dhruv-1100 wants to merge 1 commit into
Closed
fix: attribute user-triggered warnings to the caller via stacklevel#4992dhruv-1100 wants to merge 1 commit into
dhruv-1100 wants to merge 1 commit into
Conversation
Warnings a user's own code can trigger were reported at the line inside
Manim where the `logger.warning` call happens to live, giving no hint of
which line in the user's scene caused them. Pass `stacklevel` at those
call sites so the record points at the caller instead.
The correct value is per-site and was determined by inspecting the frame
chain at warning time rather than assumed:
stacklevel=3
utils/deprecation.py `decorator` inserts a wrapper frame
between the warning and user code
mobject.py / opengl_mobject.py warning lives in the private
`_insert_submobjects`, reached from
`add`, `insert` and `add_to_back`
scene.py `validate_run_time` called from play/wait/pause/wait_until
stacklevel=2
Text / MarkupText, BarChart, Surface.set_fill_by_value, Scene.embed
warning sits directly in the public
method the user calls
`Scene.replace_in_list` is deliberately left alone: it recurses over
submobjects, so no fixed stacklevel is correct for it. Warnings driven by
config or environment rather than by a user's call are also unchanged,
since a traceback there points into Manim's own plumbing.
`test_font_warnings` asserted against rendered console output, which rich
wraps to the terminal width. A longer source location shifts the wrap
point and split "not in" across two lines, so it now reads the log record
message directly, which is not width-dependent.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
|
Closing this — I opened it before properly reading the thread above. @nikolajmunk asked for this one to be left for a first-time contributor to |
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.
Overview: What does this pull request change?
Closes #4981.
Warnings that a user's own code triggers are now attributed to the user's line rather than to the line inside Manim where the
logger.warningcall lives.Before:
After:
Motivation and Explanation: Why and how do your changes improve the library?
Per @nikolajmunk's point on the issue, this is applied only where the action originates in userland — pointing a traceback at Manim's own plumbing would just add noise.
stacklevelis not a constant. It is per-call-site, and I determined each value by inspecting the actual frame chain at warning time rather than assuming:stacklevel=3utils/deprecation.py(×2)decoratorlibrary inserts a wrapper frame between the warning and user code —stacklevel=2here points atdecorator.py, not the usermobject.py,opengl_mobject.py_insert_submobjects, reached fromadd,insertandadd_to_back(verified all three entry points)scene.pyvalidate_run_timeplay/wait/pause/wait_untilstacklevel=2— warning sits directly in the public method the user calls:Text/MarkupText,BarChart,Surface.set_fill_by_value,Scene.embed(×2).Deliberately unchanged:
Scene.replace_in_list— it recurses over submobjects, so no fixedstacklevelis correct."Disabling interactive embed as dry_run is enabled", hashing, file ops, plugins, module loading). These aren't caused by a line of user code.On the feature freeze: the contributing guide notes Manim is mid-refactor and that new-feature contributions generally aren't being accepted right now, and #4981 carries the
new featurelabel. I've framed this as afixbecause the existing warnings report a misleading source location rather than because it adds capability — but if maintainers read it as a feature, I'm happy to park it until the refactor settles.I used
stacklevelrather thanstack_info=True; the latter dumps the whole stack into the log, which seemed far noisier than the issue needs. Happy to switch if you'd prefer the full trace.Further Information and Comments
One existing test needed changing.
test_font_warningsasserted against rendered console output, which rich wraps to the terminal width. Because the source location is now the (longer) caller path, the wrap point shifted and split"not in"across two lines:The warning itself is unchanged — only the rendered line breaks moved. The test now reads the log record's message via the existing
manim_caplogfixture, which isn't width-dependent.Testing. Added
tests/module/utils/test_warning_stacklevel.py, covering bothstacklevel=3shapes (decorator shim and private helper). It asserts the record'spathname,funcName, and the source text of the blamed line rather than a hardcoded line number, so it survives reformatting. Verified it fails on all four cases without the source change.tests/module/andtests/opengl/show no new failures against master (108 pre-existing failures on both, all LaTeX/Typst tests that need alatexbinary I don't have locally).🤖 Generated with Claude Code