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

Share some options between the CLI and magic. #350

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ profile something. In this case, add `PYINSTRUMENT_PROFILE_DIR = 'profiles'`
to your `settings.py`. Pyinstrument will profile every request and save the
HTML output to the folder `profiles` in your working directory.

You can further customize the filename by adding `PYINSTRUMENT_FILENAME` to
You can further customize the filename by adding `PYINSTRUMENT_FILENAME` to
`settings.py`, default values is `"{total_time:.3f}s {path} {timestamp:.0f}.{ext}"`.

If you want to show the profiling page depending on the request you can define
Expand Down
44 changes: 38 additions & 6 deletions pyinstrument/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import shutil
import sys
import time
from typing import Any, List, TextIO, cast
from typing import Any, List, Optional, TextIO, Tuple, cast

import pyinstrument
from pyinstrument import Profiler, renderers
Expand Down Expand Up @@ -412,9 +412,41 @@ def store_and_consume_remaining(
def compute_render_options(
options: CommandLineOptions, renderer_class: type[renderers.Renderer], output_file: TextIO
) -> dict[str, Any]:
"""
Given a list of CommandLineOptions, compute the
rendering options for the given renderer.

Raises if there is an error parsing the options.

"""

unicode_support: bool = file_supports_unicode(output_file)
color_support: bool = file_supports_color(output_file)

error, render_options = _compute_render_options(
options, renderer_class, unicode_support, color_support
)
if error is not None:
raise OptionsParseError(error)
assert render_options is not None
return render_options


def _compute_render_options(
options: CommandLineOptions,
renderer_class: type[renderers.Renderer],
unicode_support: bool,
color_support: bool,
) -> Tuple[Optional[str], Optional[dict[str, Any]]]:
"""
Similar as compute_render_options, but return a tuple (error message, data)
if there is an error; this will let us reuse _compute_render_options in magics.

output_file, has been replaced by unicode_support:bool, color_support:bool
"""
Comment on lines +435 to +446
Copy link
Owner

Choose a reason for hiding this comment

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

I think we don't need this extra wrapper function, rather, instead we could wrap the invocations of compute_render_options in magic.py with try:...catch:...? unless i'm missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just wanted to avoid contaminating the magic system with OptionsParseError, which is defined in __main__, and I don't want cyclic imports; but I can make compute_render_option not raise as it is used in a single place, and raise OptionsParseError in the sole place it it used.

Copy link
Contributor Author

@Carreau Carreau Jan 15, 2025

Choose a reason for hiding this comment

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

Also compute_render_options, takes an output_file while the private variant takes two booleans (unicode_support: bool, color_support: bool), as there are no output file when running the magic. I can lift both of those as well

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, the unicode/color support bit makes sense. Let's change the compute_render_options to use unicode_support and color_support (moving the file-related stuff to the call site) and then remove the _compute_render_options private version.

I'd rather do the ideomatic thing and raise exceptions on errors. I don't see a problem with magic.py importing OptionsParseError, it's already importing from __main__ anyway. But if it is an issue, we could make it a ValueError and catch that instead.

# parse show/hide options
if options.hide_fnmatch is not None and options.hide_regex is not None:
raise OptionsParseError("You can‘t specify both --hide and --hide-regex")
return ("You can‘t specify both --hide and --hide-regex", None)

hide_regex: str | None
show_regex: str | None
Expand All @@ -429,7 +461,7 @@ def compute_render_options(
options.show_all,
]
if show_options_used.count(True) > 1:
raise OptionsParseError("You can only specify one of --show, --show-regex and --show-all")
return ("You can only specify one of --show, --show-regex and --show-all", None)

if options.show_fnmatch is not None:
show_regex = fnmatch.translate(options.show_fnmatch)
Expand All @@ -449,8 +481,8 @@ def compute_render_options(
if issubclass(renderer_class, renderers.ConsoleRenderer):
unicode_override = options.unicode is not None
color_override = options.color is not None
unicode: Any = options.unicode if unicode_override else file_supports_unicode(output_file)
color: Any = options.color if color_override else file_supports_color(output_file)
unicode: Any = options.unicode if unicode_override else unicode_support
color: Any = options.color if color_override else color_support

render_options.update({"unicode": unicode, "color": color})

Expand Down Expand Up @@ -478,7 +510,7 @@ def compute_render_options(

keypath.set_value_at_keypath(render_options, key, parsed_value)

return render_options
return None, render_options


class OptionsParseError(Exception):
Expand Down
86 changes: 79 additions & 7 deletions pyinstrument/magic/magic.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,12 @@
from IPython.display import IFrame, display

from pyinstrument import Profiler, renderers
from pyinstrument.__main__ import _compute_render_options
from pyinstrument.frame import Frame
from pyinstrument.frame_ops import delete_frame_from_tree
from pyinstrument.processors import ProcessorOptions
from pyinstrument.renderers.console import ConsoleRenderer
from pyinstrument.renderers.html import HTMLRenderer

_active_profiler = None

Expand Down Expand Up @@ -76,6 +79,43 @@ def recreate_transformer(self, target_description: str):
)

@magic_arguments()
@argument(
"-p",
"--render-option",
dest="render_options",
action="append",
metavar="RENDER_OPTION",
type=str,
help=(
"options to pass to the renderer, in the format 'flag_name' or 'option_name=option_value'. "
"For example, to set the option 'time', pass '-p time=percent_of_total'. To pass multiple "
"options, use the -p option multiple times. You can set processor options using dot-syntax, "
"like '-p processor_options.filter_threshold=0'. option_value is parsed as a JSON value or "
"a string."
),
)
@argument(
"--show-regex",
dest="show_regex",
action="store",
metavar="REGEX",
help=(
"regex matching the file paths whose frames to always show. "
"Useful if --show doesn't give enough control."
),
)
@argument(
"--show",
dest="show_fnmatch",
action="store",
metavar="EXPR",
help=(
"glob-style pattern matching the file paths whose frames to "
"show, regardless of --hide or --hide-regex. For example, use "
"--show '*/<library>/*' to show frames within a library that "
"would otherwise be hidden."
),
)
@argument(
"--interval",
type=float,
Expand Down Expand Up @@ -110,6 +150,26 @@ def recreate_transformer(self, target_description: str):
nargs="*",
help="When used as a line magic, the code to profile",
)
@argument(
"--hide",
dest="hide_fnmatch",
action="store",
metavar="EXPR",
help=(
"glob-style pattern matching the file paths whose frames to hide. Defaults to "
"hiding non-application code"
),
)
@argument(
"--hide-regex",
dest="hide_regex",
action="store",
metavar="REGEX",
help=(
"regex matching the file paths whose frames to hide. Useful if --hide doesn't give "
"enough control."
),
)
@no_var_expand
@line_cell_magic
def pyinstrument(self, line, cell=None):
Expand All @@ -126,6 +186,12 @@ def pyinstrument(self, line, cell=None):
"""
global _active_profiler
args = parse_argstring(self.pyinstrument, line)

# 2024, always override this for now in IPython,
# we can make an option later if necessary
args.unicode = True
args.color = True

ip = get_ipython()

if not ip:
Expand Down Expand Up @@ -175,10 +241,19 @@ def pyinstrument(self, line, cell=None):
)
return

html_renderer = renderers.HTMLRenderer(
show_all=args.show_all,
timeline=args.timeline,
html_error, html_config = _compute_render_options(
args, renderer_class=HTMLRenderer, unicode_support=True, color_support=True
)
if html_error is not None:
raise ValueError(html_error)

text_error, text_config = _compute_render_options(
args, renderer_class=HTMLRenderer, unicode_support=True, color_support=True
)
if text_error is not None:
raise ValueError(text_error)

html_renderer = renderers.HTMLRenderer(show_all=args.show_all, timeline=args.timeline)
html_renderer.preprocessors.append(strip_ipython_frames_processor)
html_str = _active_profiler.output(html_renderer)
as_iframe = IFrame(
Expand All @@ -188,10 +263,7 @@ def pyinstrument(self, line, cell=None):
extras=['style="resize: vertical"', f'srcdoc="{html.escape(html_str)}"'],
)

text_renderer = renderers.ConsoleRenderer(
timeline=args.timeline,
show_all=args.show_all,
)
text_renderer = renderers.ConsoleRenderer(**text_config)
text_renderer.processors.append(strip_ipython_frames_processor)

as_text = _active_profiler.output(text_renderer)
Expand Down
Loading