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

set COVERAGE_CORE=sysmon to improve tests' performance in 3.12 #920

Merged
merged 1 commit into from
Feb 12, 2025

Conversation

skshetry
Copy link
Member

@skshetry skshetry commented Feb 12, 2025

Closes #906.

Running tests without the --cov option is significantly faster on Python 3.12. Similarly, the tests run quite fast on Python 3.11 with or without --cov.

It turns out that this slowdown is caused by pytest-cov, which happens due to a regression in CPython 3.12, see:

It took me a lot of time to figure out why, because this did not show up on profiling, just that everything was 3x - 4x slower. And profilers do tend to add significant overhead, so I ignored it initially.

With pytest-cov, the specific test is already running quite close to ~5s timeout, that the newer dependencies that datamodel-code-generator brought were enough to increase that to >5s, and started timing out. (For comparison, in Python 3.12, the test without --cov completes in ~1s, whereas in 3.11 with --cov, it completes in <1.5s.)

If I had led with why the test was only failing on 3.12, I might have figured this out quickly, rather than chasing dependencies, xdist and multiprocessing issues.

There is an experimental implementation inside coverage that should be faster, which can be set through COVERAGE_CORE=sysmon envvar, which will hopefully fix this issue. If not, I'll increase the timeout if necessary.

It looks like this change saves more than a minute of runtime on 3.12, and is now the fastest python version among what we test.

Before

image

After

Also worth mentioning that we do run coverage with branch=true, and it seems this could end slowing things down, but I haven't noticed that at the moment.

Copy link

codecov bot commented Feb 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.66%. Comparing base (e523ccd) to head (16f98f1).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #920   +/-   ##
=======================================
  Coverage   87.66%   87.66%           
=======================================
  Files         130      130           
  Lines       11698    11698           
  Branches     1592     1592           
=======================================
  Hits        10255    10255           
  Misses       1043     1043           
  Partials      400      400           
Flag Coverage Δ
datachain 87.58% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@skshetry skshetry marked this pull request as ready for review February 12, 2025 11:01
@skshetry skshetry changed the title set COVERAGE_CORE=sysmon to improve tests' performance in 3.12 set COVERAGE_CORE=sysmon to improve tests' performance in 3.12 Feb 12, 2025
@skshetry skshetry requested a review from a team February 12, 2025 11:01
@skshetry skshetry merged commit 72fa4b7 into iterative:main Feb 12, 2025
31 checks passed
@skshetry skshetry deleted the fix-906 branch February 12, 2025 13:42
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.

investigate why test_shutdown_on_sigterm hangs
2 participants