Skip to content

ENH: Profiler follow up#53

Merged
ZLLentz merged 6 commits into
pcdshub:masterfrom
ZLLentz:profiler-follow-up
May 18, 2022
Merged

ENH: Profiler follow up#53
ZLLentz merged 6 commits into
pcdshub:masterfrom
ZLLentz:profiler-follow-up

Conversation

@ZLLentz
Copy link
Copy Markdown
Member

@ZLLentz ZLLentz commented May 17, 2022

Description

  • Make profiler_context use a non-global profiler by default
  • Allow the global profiler to be reset
  • Be more heavy-handed in ignoring on-import errors and warnings
  • Show the unit before printing all the profiles
  • Add a min_threshold argument for omitting smaller execution times
  • Add a message for when no profiles were above the threshold, rather than just blank output

Motivation and Context

I want to keep this easy to use while not locking the use case into "call this one time only please"

How Has This Been Tested?

Interactively only

Where Has This Been Documented?

docstrings only

@ZLLentz ZLLentz requested review from klauer and tangkong May 17, 2022 18:37
@ZLLentz
Copy link
Copy Markdown
Member Author

ZLLentz commented May 17, 2022

Reverting to draft because there are more follow-ups that can be addressed!

@ZLLentz ZLLentz marked this pull request as draft May 17, 2022 18:58
@ZLLentz ZLLentz marked this pull request as ready for review May 17, 2022 23:50
@ZLLentz
Copy link
Copy Markdown
Member Author

ZLLentz commented May 17, 2022

I think this resolves everything that can be handled quickly.
The async thing will be resolved once we start running with newer versions of line_profiler.

@ZLLentz
Copy link
Copy Markdown
Member Author

ZLLentz commented May 18, 2022

On my windows box I was able to run with sys.modules as the source module list with only one minor problem: a couple modules print text on import. I didn't feel like clobbering the standard output so I left it as-is.

All of the previously problematic submodules are silently skipped.

Comment thread pcdsutils/profile.py
# I can't believe I'm catching SystemExit and preventing it.
# But this is just an import. It should be OK.
# Instead of a basic except Exception, we need to do this
# But make sure to allow a KeyBoardInterrupt
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm sorry this PR made you do such horrible things :(

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Forgive me, for I have sinned

Copy link
Copy Markdown
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

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

Looks good to me, I'm excited to use it out in the field.

Copy link
Copy Markdown
Contributor

@klauer klauer left a comment

Choose a reason for hiding this comment

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

This looks clean to me. I like the new functionality.

The only recommendation I have is that a save_results that takes a file-like object would be useful from an API standpoint. It would allow for using in-memory StringIO, easier testing, and so on.

And then I believe print_results and save_results_to_file effectively then rely on the same underlying code (as sys.stdout, sys.stderr, and open() as fp are all output streams/file-like objects)

@ZLLentz
Copy link
Copy Markdown
Member Author

ZLLentz commented May 18, 2022

Yeah, I can do that- that's how py-spy's api is under the hood too. print_results and save_results were originally just thin helpers, now they can be merged into an underlying call and made thin again. Next PR.

@ZLLentz ZLLentz merged commit a6af3a1 into pcdshub:master May 18, 2022
@ZLLentz ZLLentz deleted the profiler-follow-up branch May 18, 2022 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants