-
Notifications
You must be signed in to change notification settings - Fork 131
ENH: auto-profile stdin
or literal snippets
#338
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #338 +/- ##
==========================================
+ Coverage 63.78% 64.84% +1.05%
==========================================
Files 13 13
Lines 1041 1041
Branches 228 228
==========================================
+ Hits 664 675 +11
+ Misses 316 306 -10
+ Partials 61 60 -1 see 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
This PR looks easy to review. Can you rebase to resolve conflicts? |
33b5f5e
to
0783abf
Compare
Done! Will do the same for the other PRs. |
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.
As usual you've done great work and I only have minor comments.
kernprof.py
Outdated
# just be one inline thing (except when reading from | ||
# stdin), which can't be all that complicated | ||
RecursionError): | ||
pass |
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.
What is the purpose of parsing and unparsing? Should we output a warning if the exception case is hit?
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.
It's basically just for putting each statement onto its own line, so that we can show and profile the code more cleanly. (The caveat is that the piped-in code is lost after running the kernprof
process anyway, so it can't be python -m line_profiler
-ed and only benefits kernprof --view
).
Either way I agree, we should probably put a warning in case if the round-tripping goes south.
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.
Hmm, might it be unexpected if the user is trying to profile a specific statement and its broken up in unexpected ways? OTOH, it might be convenient to have those statements automatically broken up instead of needing to do it yourself after you realize all of the time is on a single line, and you don't know which part is causing it (something that happens a lot when I profile code normally).
But given that the input to -c can be multi-line, it might be safer to opt for the simpler case first and profile exactly what the user gave. Adding an example to the docs that demonstrates how to use this and what expected results would look like might help to decide on this point (and is probably something we want anyway).
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.
... fair enough, will do.
As for the docs thing, I realized that we kinda don't have docs for the -m
flag either (outside of the CHANGELOG
and kernprof --help
. Should we fix that too in this PR or a separate one?
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.
I tend to prioritize docs as-needed. This PR is small, so if you want to add docs for that you can do it in this PR.
For new manually written documentation, there are two ways it can be done:
- Put it in a docstring and have the autodocs handle it.
- Manually write an rst file and put it in
docs/source/manual
(maybe in the examples subfolder) and then link to it in docs/source/index.rst
For this case, because the feature isn't tied to a module it probably makes more sense to chose option 2, but there is an argument for putting it in kernprof's docstring (I do like coupling docs and code, but at some point it does get excessive).
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.
Unfortunately docs/source/manual/examples
seems to be broken – though we have
.. toctree::
:maxdepth: 8
:caption: Package Layout
auto/line_profiler
auto/line_profiler.autoprofile
auto/line_profiler.explicit_profiler
auto/kernprof
manual/examples/index
the examples aren't included in the generated doc. Will also try to fix, but I'll have to first figure sphinx
out.
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.
As long as you write the docs in there, it's good enough. If it's broken, that might be on me to fix.
You should be able to simply run make html
in the docs folder to get everything to build, and then you can check locally. I would not expect the latest URL on readthedocs to include anything from a PR.
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.
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.
It might be that the RTD integration hasn't updated itself in a while. In fact, the hash shows that it isn't: d6a2ef4 is from 2023-12-05, which is confusing because that doesn't even correspond cleanly to a release. It is a few commits after v4.1.2, so something didn't work right for the 4.1.3 or 4.2.0 release.
I don't even see the project in my RTD dashboard, so something weird is happening. I'll look into it.
EDIT: It looks like RTD updated the webhooks on their end, so probably none of my stuff is getting updated. I think I was able to fix it, and the latest now shows a more recent version of the docs.
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.
kernprof.py
Outdated
import ast | ||
import tempfile | ||
|
||
source, content = tempfile_source_and_content |
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.
We should probably call textwwrap.dedent on the code. This feature will be part of CPython in 3.14: python/cpython#103998
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 catch, thanks. This reminds me, I should probably get 3.14 and play around with it.
Thanks for the review! Just implemented all the suggestions. |
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.
In addition to the bashisms I specifically mention, I'm on the fence about using the PS1/PS2 prompt strings to separate code from output. On the one hand, it's standard and I see it everywhere. On the other hand, I hate it; It prevents the user from just copy/pasting the code into the terminal and trying it out themselves.
This is definitely one of my personal soapboxes, and its very much an opinion. But at the same time, I know I have other documentation which actively avoids it, so it would be inconsistent to have both in the same code base.
At the same time, the my alternative has issues. I usually do things like:
Run this:
echo "hello world"
and the output is
hello world
which isn't nearly as clean looking as:
$ echo "hello world"
hello world
So on one hand, the style I hate looks better in docs, but on the other hand it hinders user interaction, and requires precise mouse selection that my old hands scream about. I'd be interested in any brainstorming on potentially better ways to do this.
$ code="import sys; " | ||
$ code+="from fib import _run_fib, fib_no_cache as fib; " | ||
$ code+="for n in sys.argv[1:]: print(f'fib({n})', '=', fib(int(n)))" | ||
$ kernprof --prof-mod fib._run_fib --line-by-line --view -c "${code}" 10 20 |
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.
$ code="import sys; " | |
$ code+="from fib import _run_fib, fib_no_cache as fib; " | |
$ code+="for n in sys.argv[1:]: print(f'fib({n})', '=', fib(int(n)))" | |
$ kernprof --prof-mod fib._run_fib --line-by-line --view -c "${code}" 10 20 | |
$ kernprof --prof-mod fib._run_fib --line-by-line --view -c " | |
import sys | |
from fib import _run_fib, fib_no_cache as fib | |
for n in sys.argv[1:]: | |
print(f'fib({n})', '=', fib(int(n))) | |
" 10 20 |
I strongly recommend just using newlines in your invocation of -c, rather than setting up a convoluted semicolon separated set of statements in a bash variable. (This is the reason dedent is important for -c arguments, although previously you could do something like:
python -c "if 1:
for i in range(2):
print('hello multiline world')
"
Furthermore, this example does not work. In your version I get:
File "/tmp/tmprybp7qo7/kernprof-command.py", line 1
import sys; from fib import _run_fib, fib_no_cache as fib; for n in sys.argv[1:]: print(f'fib({n})', '=', fib(int(n)))
^^^
SyntaxError: invalid syntax
And in my version I get:
File "/tmp/tmphmw5r33x/kernprof-command.py", line 3, in <module>
ModuleNotFoundError: No module named 'fib'
Is that one of the issues addressed by a different PR?
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.
Yep I did mess up with this example; will fix as suggested.
It's probably not related to any PRs though, since your ModuleNotFoundError
is just a result of not having ${PWD}
in ${PYTHONPATH}
(which as stated in the intro of this page is assumed by the examples). Will add a PYTHONPATH="${PYTHONPATH}:${PWD}"
to make it more compatible EDIT: I meant portable.
$ read -d '' -r code <<-'!' | ||
> from fib import fib | ||
> | ||
> def my_func(n=50): | ||
> result = fib(n) | ||
> print(n, '->', result) | ||
> | ||
> my_func() | ||
> ! | ||
$ kernprof -lv -c "${code}" |
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.
$ read -d '' -r code <<-'!' | |
> from fib import fib | |
> | |
> def my_func(n=50): | |
> result = fib(n) | |
> print(n, '->', result) | |
> | |
> my_func() | |
> ! | |
$ kernprof -lv -c "${code}" | |
$ kernprof -lv -c " | |
from fib import fib | |
def my_func(n=50): | |
result = fib(n) | |
print(n, '->', result) | |
my_func() | |
" |
Would read better without bashisms.
$ kernprof --prof-mod fib._run_fib --line-by-line --view - 10 20 <<-'!' | ||
> import sys | ||
> from fib import _run_fib, fib_no_cache as fib | ||
> for n in sys.argv[1:]: | ||
> print(f"fib({n})", "=", fib(int(n))) | ||
> ! |
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.
$ kernprof --prof-mod fib._run_fib --line-by-line --view - 10 20 <<-'!' | |
> import sys | |
> from fib import _run_fib, fib_no_cache as fib | |
> for n in sys.argv[1:]: | |
> print(f"fib({n})", "=", fib(int(n))) | |
> ! | |
$ kernprof --prof-mod fib._run_fib --line-by-line --view - 10 20 <<-'!' | |
$ echo ' | |
import sys | |
from fib import _run_fib, fib_no_cache as fib | |
for n in sys.argv[1:]: | |
print(f"fib({n})", "=", fib(int(n))) | |
' | kernprof --prof-mod fib._run_fib --line-by-line --view - 10 20 |
Debatable which one is better here.
Regarding what you said about separating Bash/shell inputs from outputs, I wonder if interlacing normal (input) and collapsible (output) code blocks helps in delineating the two and keeping thing relatively compact? Thanks for the comments, will take a closer look after dinner. |
The collapsible output looks like a nice way to handle it. |
CHANGELOG.rst Edited entry kernprof.py __doc__ Updated main() - Updated function used to find name of the Python executable to be more lenient in abbreviating the name, requiring file identity (via `os.path.samefile()`) instead of string-path equality - Added new option `-c`, which causes the positional argument to be interpreted as an inline script (as with `python -c`) instead of the path to a script file - Added special case for `script = -` to read the script to profile from stdin
tests/test_autoprofile.py test_autoprofile_from_{stdin,inlined_script}() New tests for `kernprof -c ...` and `kernprof -`; note that it isn't necessary to separately test for the non-line-by-line modes, because only the new parts (tempfile-writing) need to be tested
kernprof.py - Replaced `tempfile.NamedTemporaryFile` with `tempfile.TemporaryDirectory` (helps with cross-platform compatibility and performance in some edge cases) - Fixed bug where the tempfile name is split info chars for `--prof-mod` test/test_autoprofile.py test_autoprofile_from_stdin() Added subtests which test the interactions with the `-p` and `-v` flags test_autoprofile_from_inlined_script() Cleaned up test body
kernprof.py main() - Streamlined code path to print the help text and exit - Refactored the writing of stdin/`-c` arguments to tempfiles info its own function () so that the `return` statements are closer together _write_tempfile() Added `textwrap.dedent()`-ing to the provided source code
kernprof.py __doc__ Added note on `-c`, `-m`, and `-` _write_tempfile(), _main() Added basic docstrings because private methods are also shown in the auto-generated docs _write_tempfile() Removed AST round-tripping
docs/source/manual/examples/example_kernprof.rst - Removed assumption that `${PWD}` is in `${PYTHONPATH}` - Replaced `console` code blocks with `bash` code blocks + collapsible output code blocks - Reworked examples in "Literal-code execution": replaced concatenation/`read` into shell variable with multiline strings - Reworked example in "Executing code read from ``stdin``": replaced heredoc with command pipeline
06ed818
to
96047f4
Compare
CHANGERLOG.rst - Reworded previous entry (pyutils#338) - Reworded entry kernprof.py __doc__ Updated _normalize_profiling_targets.__doc__ _restore_list.__doc__ pre_parse_single_arg_directive.__doc__ Reformatted to be more `sphinx`-friendly main() - Removed the `-e`/`--eager-preimports` flag - Made eager pre-imports the default for `--prof-mod` - Added new flag `--no-preimports` for restoring the old behavior
This PR is inspired by #323, which added the
-m
option tokernprof
so that one can profile the execution of modules and packages. Much like howkernprof <script>
parallelspython <script>
, andkernprof -m <module>
python -m <module>
, there may be use-cases that call for similar parallels for:python -c <code>
(running literal code), and... | python -
orpython - <<<"..."
(running code fromstdin
).Hence this PR which adds to
kernprof
:-c
(like how-m
is implemented), which terminates the parsing of options and runs the following argument as literal Python code, and-
, which promptskernprof
to read fromstdin
.It is implemented by a minor refactoring, which writes the received code to a temporary file and profiles that instead.
2 tests are also added:
test_autoprofile_from_stdin()
: test for... | kernprof -
.test_autoprofile_from_inlined_script()
: test forkernprof -c <code>
.EDIT 6 May: added documentations on the
-m
,-c
, and-
invocations forkernprof
.