-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
gh-82054: allow test runner to split test_asyncio #103859
Conversation
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.
Looks good! Just a couple comments.
Misc/NEWS.d/next/Tests/2023-04-25-23-56-04.gh-issue-gh-82054.3GgB66.rst
Outdated
Show resolved
Hide resolved
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
@zitterbewegung in order to make the case for landing this, it would be useful to update the PR description with some current data. Copying the description from the Cinder diff isn't really useful, since it's from a much older version. What would be helpful is to run It also may be that test_asyncio is no longer the longest pole, and we may need to apply the same treatment to some other modules (test_multiprocessing?) in order to see a significant benefit in overall runtime of |
@carljm I can see an overall benefit of 100 seconds from running cpython's baseline. I have attached both runs. This was executed on a AMD Ryzen Threadripper 3970X 32-Core Processor with 128GB of ram. |
@zitterbewegung Awesome, those are great results! Can you directly update the PR description/summary to eliminate the text copied from the Cinder diff and instead just summarize these results? |
Updated summary with new results. |
@zitterbewegung Ok, one last thing I see is that |
Done. |
I ran make patch check after I did the changes |
Summary:
This runs test_asyncio sub-tests in parallel using sharding by cinder. These two tests are typically the long-poles in runs because they are modules with a lot of further sub-tests run serially. By breaking out the sub-tests as independent modules we can run a lot more in parallel.
After porting we can see the direct impact is extremely large (15% increase in performance):
The drawbacks are that this implementation is hacky and due to the sorting of the tests it obscures when the asyncio tests occur and involves changing CPython test infrastructure but, the time saved it is worth it . It's not a complicated change and I think the win in productivity with the change above is significant.