gh-142927: Auto-open HTML output in browser after generation by ivonastojanovic · Pull Request #143178 · python/cpython
You probably need to update the test to NOT open the browser by default and make the CLI the only that sets that option to True :)
pablogsal
changed the title
gh-138122: Auto-open HTML output in browser after generation
gh-142927: Auto-open HTML output in browser after generation
Some tests re opening browsers now 😆
You probably want this diff at least:
diff --git a/Lib/test/test_profiling/test_sampling_profiler/test_children.py b/Lib/test/test_profiling/test_sampling_profiler/test_children.py index b7dc878a238..3d6db7dcb56 100644 --- a/Lib/test/test_profiling/test_sampling_profiler/test_children.py +++ b/Lib/test/test_profiling/test_sampling_profiler/test_children.py @@ -992,6 +992,7 @@ def test_subprocesses_flag_with_flamegraph_output(self): "-r", "100", "--flamegraph", + "--no-browser", "-o", output_file, script_file,
Hey, 👋
sorry for a drive-by comment, just wanted to chime in with a specific use case: the default behaviour you're introducing here is suuuper annoying when being logged in via SSH (and based on my experience it's too easy to forget the --no-browser option 🙈).
According to this post, it might be possible (at least on best effort basis) to check some environment variables to detect the SSH connection, and perhaps disable the browser-opening automatically in that case?
If that sounds reasonable I can do a bit of testing for SSH detection tomorrow. The biggest disadvantage I see that it would make the default behaviour less trivial to explaing to users...
Hey, 👋
sorry for a drive-by comment, just wanted to chime in with a specific use case: the default behaviour you're introducing here is suuuper annoying when being logged in via SSH (and based on my experience it's too easy to forget the
--no-browseroption 🙈).According to (this post)[https://unix.stackexchange.com/a/9607], it might be possible (at least on best effort basis) to check some environment variables to detect the SSH connection, and perhaps disable the browser-opening automatically in that case?
If that sounds reasonable I can do a bit of testing for SSH detection tomorrow. The biggest disadvantage I see that it would make the default behaviour less trivial to explaing to users...
Hey! After thinking about it a bit more, I agree with you 🙂 It does seem more annoying to forget to set --no-browser than to just have the behavior disabled by default and turn it on with --browser when we actually need it. I’ll go ahead and change the default to false, having it enabled by default feels easier to forget and more likely to cause issues in these cases.
Add a --browser flag that automatically opens the generated flamegraph and heatmap HTML files in the default browser once profiling completes.
Another, somewhat middle-ground, option would be to have a prompt to open a browser; this is only one or two keystrokes less convenient than launching the browser automatically, but avoids the pitfalls mentioned above. This is what we do in one of our CLI apps and I quite like it, but I know having prompts might be controversial for some.
(in our case, one can pass --no-browser to disable the prompt)
I wonder if printing a URL that one can copy-paste would work in this case as well?
btw: I love what you guys are doing with the new profiler, can't wait to try it out. 🫶 🤘
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Given the restrictions i think for now the best course of action is making it opt-in! Great work @ivonastojanovic
thunder-coding pushed a commit to thunder-coding/cpython that referenced this pull request