-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-5404 - Add docs for profiling execution #2402
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
Conversation
tools/generate_flamegraph.py
Outdated
import sys | ||
|
||
|
||
def main(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth having this file at all? We're adding 60 lines of code to change this:
pip install py-spy
py-spy record -o output.svg -- <path/to/script>
into:
pip install py-spy
just flamegraph <--output_name=profile> <--sample_rate=2000> <path/to/script>
Can we just pass all the args to py-spy record
and call it a day? It already has a default value with sample_rate so all we really save here is the -o profile.svg
which I'm not sure is worth this extra file. The just flamegraph
approach is also inflexible since it doesn't allow passing arbitrary options to py-spy record
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I like the conciseness of just flamegraph
, but we're already requiring enough args and manual steps that we don't save much at all by having it. Just including docs on how to run py-spy should be sufficient here.
CONTRIBUTING.md
Outdated
2. Inside your test script, perform any required setup and then loop over the code you want to profile for improved sampling | ||
3. Run the `flamegraph` justfile target to generate a `.svg` file containing the flame graph: | ||
```bash | ||
just flamegraph <output_name=profile> <sample_rate=2000> <path/to/script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we even add this to just
? Since we already need to install py-spy
manually why not document the py-spy command to run without just
.
Thanks! |
No description provided.