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

Use process_time instead of just time for measuring test performance. #1413

Closed

Conversation

mergezalot
Copy link
Contributor

I hope this removes a handful of random test fails on supposedly busy cloud runners. At least it is worth a try.

thread_time https://docs.python.org/3/library/time.html#time.thread_time could help with parallel test runs, too, but that only is available since Python 3.7.

process_time is available since Python 3.3.

This PR is a follow up to

…nce.

I hope this removes a handful of random test fails on supposedly busy cloud runners.

`thread_time` https://docs.python.org/3/library/time.html#time.thread_time could help
with parallel test runs, too, but that only is available since Python 3.7.

process_time is available since Python 3.3.
@codecov
Copy link

codecov bot commented Oct 30, 2022

Codecov Report

Base: 94.19% // Head: 94.19% // No change to project coverage 👍

Coverage data is based on head (e73032e) compared to base (613b370).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1413   +/-   ##
=======================================
  Coverage   94.19%   94.19%           
=======================================
  Files          28       28           
  Lines        5085     5085           
  Branches      968      968           
=======================================
  Hits         4790     4790           
  Misses        176      176           
  Partials      119      119           
Impacted Files Coverage Δ
PyPDF2/_page.py 92.16% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@pubpub-zz
Copy link
Collaborator

This is a Just in time fix!
I faced the issue about one hour ago pushing my PR : will use your solution😍

pubpub-zz added a commit to pubpub-zz/pypdf that referenced this pull request Oct 30, 2022
@pubpub-zz
Copy link
Collaborator

@mergezalot,
I've tried your solution but still not good on my PR under Python 11. Any ideas?

@mergezalot
Copy link
Contributor Author

mergezalot commented Oct 31, 2022

@pubpub-zz educated guess: either parallel tests, super busy runners, or low performance runners.

I tried a different approach now:
Use thread_time on python > 3.7. Skip otherwise. If that still fails, I suggest we drop my test. It is not that likely to catch future problems, and it has many false positives.

MartinThoma added a commit that referenced this pull request Nov 1, 2022
The test before was to brittle. We need to keep an open eye to the
benchmarks in future, but also be careful with interpreting the
numbers.

Credits to mergezalot in PR #1413
MartinThoma added a commit that referenced this pull request Nov 1, 2022
The test before was to brittle. We need to keep an open eye to the
benchmarks in future, but also be careful with interpreting the
numbers.

Credits to mergezalot in PR #1413
@MartinThoma
Copy link
Member

I've added this test as a benchmark: https://py-pdf.github.io/PyPDF2/dev/bench/

In the following test you can see that the performance is very different from run to run:

image

You will get one datapoint for every future commit in main:

Untitled

@MartinThoma
Copy link
Member

@mergezalot Thank you for your work on this topic. time.process_time was new to me 🙏

However, I think timing is only suitable for distinguishing orders of magnitude (e.g. as for test_do_not_get_stuck_on_large_files_without_start_xref introduced in #808 . There the difference was more than 5 minutes for the old solution vs less than a second for the new one. @dsk7 chose to make the test fail if the time is over one minute - that worked so far very well.

@MartinThoma
Copy link
Member

Would it be ok to close this PR?

@mergezalot
Copy link
Contributor Author

@MartinThoma yes, let's close this PR. Benchmarking is much better. Thank you.

@mergezalot mergezalot closed this Nov 1, 2022
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