Skip to content

bpo-37937: Mention frame.f_trace in sys.settrace docs #15439

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

Merged
merged 4 commits into from
Sep 20, 2019

Conversation

cool-RR
Copy link
Contributor

@cool-RR cool-RR commented Aug 23, 2019

After I released PySnooper, @alexmojaki added the awesome optimization of using f_trace instead of sys.settrace. He needed to explain to me how it works, because I couldn't find any documentation of f_trace online. So I figured I'd add it to Python's documentation.

https://bugs.python.org/issue37937

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

@cool-RR
Copy link
Contributor Author

cool-RR commented Aug 24, 2019

@brandtbucher Do you think this PR qualifies for the "skip issue" and "skip news" labels? If it's no on the latter I can add a news entry, let me know.

@alexmojaki
Copy link

I think there's still some confusion. f_trace is not being used as an optimisation in PySnooper. It's there for correctness, not performance. Specifically it makes with snoop(): actually trace the frame that it's in, since sys.settrace() can't. Using it as an optimisation instead of just sys.settrace() would be very difficult because you generally want to start tracing frames from the beginning of their execution but it's hard to obtain a frame anywhere but from within it. Are you using this in a project of your own like that?

@cool-RR
Copy link
Contributor Author

cool-RR commented Aug 24, 2019 via email

@alexmojaki
Copy link

I think it's important to mention (though I'm not sure of the best way to word it) that if you implement a debugger and you provide some function to 'start tracing here' like a breakpoint (e.g. pdb.set_trace or pysnooper.Tracer.__enter__), then you need to set f_trace on the frame where that function was called. sys.settrace() will never do that on its own because it only starts working when you enter a new scope. That is the thing that confused and bothered me when I was implementing the functionality in both PySnooper and another project of mine, and I had to look at pdb's source to figure out what was going on.

@alexmojaki
Copy link

Here's a suggestion:

Note when implementing a function which acts like a breakpoint and starts tracing at the point where it was called (e.g. pdb.set_trace()): sys.settrace() only starts tracing when it first enters a new scope. To start tracing immediately from your debugger's own function, obtain the frame where the function was called and set frame.f_trace = tracefunc in addition to sys.settrace(tracefunc).

@brandtbucher
Copy link
Member

brandtbucher commented Aug 24, 2019

@cool-RR Given that this isn’t a trivial doc fix, I’d still include a basic NEWS entry.

@cool-RR cool-RR changed the title Mention frame.f_trace in sys.settrace docs bpo-37937: Mention frame.f_trace in sys.settrace docs Aug 24, 2019
@cool-RR
Copy link
Contributor Author

cool-RR commented Aug 24, 2019

Added to NEWS, created a bpo ticket. Let me know whether anything else is missing.

@terryjreedy
Copy link
Member

With help from 2 non-coredev reviews, this looks ready for a coredev to review, and if the addition is correct, likely merge. This is too far from my areas of expertise to know. If the addition applies to 3.8 and 3.7, add backport labels. I suggest that one of you peruse the experts list and select a couple to request.

@cool-RR
Copy link
Contributor Author

cool-RR commented Aug 31, 2019

I'd like to add the backport label for 3.7 and 3.8, but I'm not seeing a way to do that. Can anyone help?

@cool-RR
Copy link
Contributor Author

cool-RR commented Aug 31, 2019

@benjaminp @pitrou I'm tagging you guys because I saw you touched f_trace in the past. Would any of you care to review this PR?

@cool-RR
Copy link
Contributor Author

cool-RR commented Sep 4, 2019

I guess @benjaminp and @pitrou are busy. Maybe @ncoghlan or @serhiy-storchaka would like to review this?

@cool-RR
Copy link
Contributor Author

cool-RR commented Sep 6, 2019

Paging @abalkin , maybe you'd like to review this?

@ncoghlan
Copy link
Contributor

Belongs to only in the sense of "I think I'm the one that touched it last" ;)

The f_trace frame attribute is certainly intended to be read/write, it's just that the conventional way of setting it is indirectly via the return value from a globally installed trace function.

@cool-RR
Copy link
Contributor Author

cool-RR commented Sep 20, 2019

Thanks for chiming in, Nick. Are you saying that because of your statement above, you disagree with this addition to the docs?

Copy link
Contributor

@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

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

This mostly looks excellent, thanks - just one point that I think could be confusing if we don't adjust the wording slightly.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@cool-RR
Copy link
Contributor Author

cool-RR commented Sep 20, 2019

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@ncoghlan: please review the changes made to this pull request.

Copy link
Contributor

@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

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

Thanks! There was one further amendment needed (related to exactly what's required to enable the C level trace hook that calls frame.f_trace), which I went ahead and made after double-checking the exact requirements in the interactive REPL.

@ncoghlan ncoghlan merged commit 9c2682e into python:master Sep 20, 2019
@miss-islington
Copy link
Contributor

Thanks @cool-RR for the PR, and @ncoghlan for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-16296 is a backport of this pull request to the 3.8 branch.

@bedevere-bot
Copy link

GH-16297 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 20, 2019
Mention frame.f_trace in sys.settrace docs, as well as the fact you still
need to call `sys.settrace` to enable the tracing machinery before setting
`frame.f_trace` will have any effect.
(cherry picked from commit 9c2682e)

Co-authored-by: Ram Rachum <ram@rachum.com>
miss-islington added a commit that referenced this pull request Sep 20, 2019
Mention frame.f_trace in sys.settrace docs, as well as the fact you still
need to call `sys.settrace` to enable the tracing machinery before setting
`frame.f_trace` will have any effect.
(cherry picked from commit 9c2682e)

Co-authored-by: Ram Rachum <ram@rachum.com>
@cool-RR
Copy link
Contributor Author

cool-RR commented Sep 20, 2019

Wheee 😊

And thanks to @alexmojaki for the guidance.

miss-islington added a commit that referenced this pull request Sep 20, 2019
Mention frame.f_trace in sys.settrace docs, as well as the fact you still
need to call `sys.settrace` to enable the tracing machinery before setting
`frame.f_trace` will have any effect.
(cherry picked from commit 9c2682e)

Co-authored-by: Ram Rachum <ram@rachum.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants