Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Gracefully error out when there are concurrent profilers #692

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

albertyw
Copy link
Member

Fixes #682
Unblocks #686

Copy link

codecov bot commented Dec 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (23ff43b) 86.65% compared to head (6537b6e) 86.67%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #692      +/-   ##
==========================================
+ Coverage   86.65%   86.67%   +0.02%     
==========================================
  Files          52       52              
  Lines        2113     2117       +4     
==========================================
+ Hits         1831     1835       +4     
  Misses        282      282              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@albertyw albertyw force-pushed the concurrent-profiler branch 4 times, most recently from f0b8b96 to e4e8653 Compare December 30, 2023 23:05
@albertyw albertyw force-pushed the concurrent-profiler branch from b42e593 to 6537b6e Compare December 30, 2023 23:06
@albertyw albertyw mentioned this pull request Dec 31, 2023
Copy link
Member

@50-Course 50-Course left a comment

Choose a reason for hiding this comment

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

Hello again @albertyw,

this patch permanently fixes the Django==5.1 & python^=3.12 pipeline issue?

Copy link
Member

@50-Course 50-Course left a comment

Choose a reason for hiding this comment

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

Update: Great progress! The "Gracefully error out when there are concurrent profilers" job (#722) is now passing, which has also unblocked #686.

@50-Course 50-Course merged commit 1367d58 into jazzband:master Jan 8, 2024
@robinchow
Copy link
Contributor

I'd like to point out while this stop silk from interfering with processing a request, it also ends up not profiling that particular request. Prior to Python 3.11, the effect was similar but different (it would disable/stop the first profiler, and then start this current one). Neither outcome seems great, especially since this problem will happen when Django is processing multiple long-running requests at the same time (I've made a minimal repo here to highlight this issue)

I think at a minimum, the documentation should tell users that this issue exists. In terms of a proper fix, I'm not sure at the moment if that's possible without using another profiler that properly supports multithreaded applications (such as https://github.com/sumerc/yappi)

@50-Course
Copy link
Member

Thank you for pointing out this oversight, @robinchow. I'd take a look into yappi

@albertyw
Copy link
Member Author

Prior to Python 3.11, the effect was similar but different (it would disable/stop the first profiler, and then start this current one)

3.11 or 3.12? This code gives an error in python 3.12 but no error in 3.11:

import cProfile
p1 = cProfile.Profile()
p1.enable()
p2 = cProfile.Profile()
p2.enable()

@robinchow
Copy link
Contributor

@albertyw ah yes, apologies, prior to Python 3.12.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Python 3.12] ValueError: Another profiling tool is already active
3 participants