Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[better_errors] Ensure that tracer errors in for_loop point to use code #25798

Merged
merged 1 commit into from
Jan 14, 2025

Conversation

gnecula
Copy link
Collaborator

@gnecula gnecula commented Jan 9, 2025

Fixes: #23637

@gnecula gnecula self-assigned this Jan 9, 2025
@gnecula gnecula added the pull ready Ready for copybara import and testing label Jan 9, 2025
@gnecula gnecula requested review from jakevdp and mattjj January 9, 2025 11:28
@gnecula gnecula changed the title [better_errors] Ensure that tracer errors in for_loop points to use code [better_errors] Ensure that tracer errors in for_loop point to use code Jan 9, 2025
@gnecula gnecula force-pushed the fix_fori_error branch 2 times, most recently from 1f0ed9f to 7bdeb19 Compare January 13, 2025 13:01
@gnecula gnecula requested a review from jakevdp January 13, 2025 13:01
body_fun = weakref.ref(body_fun)
body_fun_ref = weakref.ref(body_fun)

@wraps(body_fun)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, looking more closely here, we probably can't use this mechanism (either functools.wraps or functools.update_wrapper) because it results in a strong reference of the original function being attached to the wrapped function: https://github.com/python/cpython/blob/aa6579cb60b4fcd652d7bc83fed2668e4ae84db3/Lib/functools.py#L61

Copy link
Collaborator Author

@gnecula gnecula Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, then the solution is to inline (parts of) wraps to get the function name to work properly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the end, I decided to just save the fun_sourceinfo, rather than saving all the ingredients (the filename and lineno are read-only).

@gnecula gnecula requested a review from jakevdp January 13, 2025 15:35
@copybara-service copybara-service bot merged commit 4f2f5fa into jax-ml:main Jan 14, 2025
23 checks passed
@gnecula gnecula deleted the fix_fori_error branch January 14, 2025 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull ready Ready for copybara import and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

when a tracer error happens in for_loop, should point to the user's body function
2 participants