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

CI Use sys.monitoring with coverage to speed-up Python >= 3.12 builds #29473

Merged
merged 18 commits into from
Dec 2, 2024

Conversation

lesteve
Copy link
Member

@lesteve lesteve commented Jul 12, 2024

Follow-up of #29444 (comment), trying to see if using statement coverage makes a difference.

ref statement coverage + sysmon + pylatest_pip_openblas_pandas to Python 3.12
pylatest_conda_forge_mkl (Python 3.12) 24m 12s 25m 23s 14m 17s 13m 56s
pymin_conda_forge_openblas_ubuntu_2204 (no coverage) 19m 19s 11m 19s 12m 54s 15m 26s
ubuntu_atlas (no coverage) 16m 16 15m 42s 11m 15s 16m 50s
pymin_conda_defaults_openblas 20m 14m 6s 14m 6s 13m 41s
pylatest_pip_openblas_pandas (Python 3.11 or 3.12) 27m 9s 35m 19s 31m 53s 22m 19s
debian_atlas_32bit 34m 42s 34m 5s 46m 31s 44m 23s
macOS pylatest_conda_forge_mkl (Python 3.12) 19m 11s 15m 49s 10m 45s 11m 16s
macOS pylatest_conda_mkl_no_openmp (Python 3.12) 20m 57s 18m 53s 18m 35s 25m 3s
Windows pymin_conda_forge_mkl (no coverage) 18m 59s 24m 10s 20m 13s 32m 2s

Reference run (same as main) build log
image

Run with using statement coverage rather than branch coverage build log
image

Run with using statement coverage + COVERAGE_CORE=sysmon, this should make Python 3.12 builds faster build log
image

Run with using statement coverage + COVERAGE_CORE=sysmon, this should make Python 3.12 builds + updating openblas_pandas to Python 3.12 build log
image

Copy link

github-actions bot commented Jul 12, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: a3d3a8d. Link to the linter CI: here

@thomasjpfan
Copy link
Member

@lesteve Have you experimented with https://github.com/plasma-umass/slipcover on this repo before?

The author did run benchmarks with scikit-learn's testing: https://github.com/plasma-umass/slipcover?tab=readme-ov-file#performance I tried slipcover ~2 years ago and did see some improvements, but did not have time to purse it.

@lesteve
Copy link
Member Author

lesteve commented Jul 22, 2024

@lesteve Have you experimented with plasma-umass/slipcover on this repo before?

I have heard of it recently but not run anything. It seems like it is missing a few integration that we need e.g. pytest-xdist support, XML results file.

There are some discussions about merging SlipCover and coverage according to this slide of the PyConUS 2024 SlipCover talk.

coveragepy with Python 3.12 can use sys.monitoring which is maybe as fast as SlipCover, at least this is what I gathered from the slides after this one but not 100% sure.

Right now coveragepy does not really support branch coverage with COVERAGE_CORE=sysmon, it actually makes it slower see nedbat/coveragepy#1812. To be honest I don't think we care that about branch coverage vs statement coverage and this PR was a way to test how much using statement coverage for all the builds and COVERAGE_CORE=sysmon for the Python 3.12 build would help making the CI faster.

The summary so far:

  • using statement coverage vs branch coverage maybe is a bit faster (for example the pymin_conda_defaults_openblas build goes from 20 minutes to 14 minutes)
  • using statement coverage + COVERAGE_CORE=sysmon for the Python 3.12 is definitely faster
  • some build timings are quite variable so take this with a pinch of salt. For example Windows which have coverage disabled and should not care about all of the different settings varies between 19 minutes and 32 minutes

@ogrisel
Copy link
Member

ogrisel commented Aug 2, 2024

Could you please trigger another run for debian_atlas_32bit with statement coverage + sysmon? The 44 min runtime is a bit worrying. I wonder if it's random or not.

@lesteve
Copy link
Member Author

lesteve commented Aug 3, 2024

The 44 min runtime is a bit worrying.

Could you please trigger another run for debian_atlas_32bit with statement coverage + sysmon? The 44 min runtime is a bit worrying. I wonder if it's random or not.

Note that in a67708e, Debian 32bit build took 35 minutes see build log.

I think since we upgraded the Debian version for the 32bit CI (11->12) this build is a lot slower with quite some variability (the Debian 11->12 upgrade was done when getting rid of setuptools PR #29400). My hunch is that this is due to upgrading Python to 3.11 and that this might be related to coverage too but I am not 100% sure.

For example on main, you can easily find some Debian 32bit runs that took ~45 minutes e.g. this one or this one. To find them you look at all the recent runs on main and pick the ones that take more than 1h.

@ogrisel
Copy link
Member

ogrisel commented Nov 6, 2024

@lesteve this PR is still draft but I have the impression that it's a net improvement, no?

@lesteve
Copy link
Member Author

lesteve commented Nov 13, 2024

@lesteve this PR is still draft but I have the impression that it's a net improvement, no?

It does speed up Python >= 3.12 builds. The minor downside is that we only use statement coverage which is less strict that branch coverage.

With statement coverage the code below is fully covered. With branch coverage, the if x: line would be shown as partially covered.

def func(x):
    if x:
        y = 'defined'
    print(x)

func(True)

@lesteve
Copy link
Member Author

lesteve commented Nov 26, 2024

I pushed 3 commits and honestly it seems to be worth it now, for example the current bottleneck debian32 takes 25 minutes instead of 50-60 minutes, I am marking this as ready for review.

To reiterate the minor downside is that statement coverage is not as strict as branch coverage, see #29473 (comment).

Here is a summary table:

time saved by this PR ref main PR run 1 PR run 2 PR run 3
pylatest_conda_forge_mkl (Python 3.13) 7-10m 🔥 23m42s 12m29s 12m34s 15m54s
pymin_conda_forge_openblas_ubuntu_2204 (no coverage) N/A 15m27s 18m54s 13m57s 12m29s
ubuntu_atlas (no coverage) N/A 17m40s 16m11s 13m5s 14m15s
pymin_conda_defaults_openblas (Python 3.9) maybe slower by 6m? 🤔 18m41s 25m2s 25m5s 25m49s
pylatest_pip_openblas_pandas (Python 3.11 on main, 3.13 in PR) none or ~10m once PR merged 33m2s 41m51s 34m15s 32m26s
debian_atlas_32bit (Python 3.12) 20-30m 🔥🔥🔥 49m58s 26m18s 26m17s 26m12s
macOS pylatest_conda_forge_mkl (Python 3.13) 10-13m 🔥🔥 26m58s 13m42s 14m10s 15m43s
macOS pylatest_conda_mkl_no_openmp (Python 3.12) 10-18m 🔥🔥 33m4s 15m52s 14m53s 23m13s
Windows pymin_conda_forge_mkl (no coverage) N/A 23m0s 23m8s 23m6s 25m20s

Timings from build logs for first commit, second commit, third commit

Comments:

  • debian32 is 25-30minutes faster. This is the bottleneck these days now uses Python 3.12 (was Python 3.11 until not so long ago) so it is a lot faster 26minutes compared to 50-60 minutes on main, look at Azure CI builds on main to compare.
  • pylatest_pip_openblas is still a bit long ~35 minutes but I am pretty sure part of it is that Python is upgraded to Python 3.13 and so we don't have any ccache cache so the install takes 10-15 minutes instead of 5.
  • the pymin_conda_forge_openblas may be a bit slower for some reason ~6minutes, maybe some variability in the CI builds, it probably does not matter that much compared to the other gains

@lesteve lesteve marked this pull request as ready for review November 26, 2024 08:43
@lesteve lesteve changed the title CI Use statement coverage rather than branch coverage CI Use sys.monitoring with coverage to speed-up Python >= 3.12 builds Nov 26, 2024
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Thank you for the analysis~ I'm happening with using sys.monitoring. LGTM

Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM !

@jeremiedbb
Copy link
Member

You need to regenerate the lock files though

@jeremiedbb jeremiedbb merged commit fba028b into scikit-learn:main Dec 2, 2024
30 checks passed
@lesteve lesteve deleted the statement-coverage branch December 3, 2024 05:47
@lesteve
Copy link
Member Author

lesteve commented Dec 3, 2024

Note: I found out that one reason that makes the pylatest_pip_openblas_pandas is that there is no scikit-image and pyamg wheel so we build them from source ... for example this build on main takes ~15 minutes but the ccache show a reasonable hit rate and also 3min30s between stats zeroed and stats updated so I guess that means that scikit-learn build took ~3min30s. I also had a quick look at the raw log with timestamps on each line that seem to indicate that about 10 minutes is spent on creating the environment which would do pip install scikit-image and build from source.

Hopefully there will be a scikit-image with Python 3.13 wheel soonish (there was a rc in October 2024 with Python 3.13 wheels).

scikit-image and pyamg are the only .tar.gz in our lock-files currently:

❯ git grep tar.gz build_tools       
build_tools/azure/pylatest_pip_openblas_pandas_linux-64_conda.lock:# pip pyamg @ https://files.pythonhosted.org/packages/72/10/aee094f1ab76d07d7c5c3ff7e4c411d720f0d4461e0fdea74a4393058863/pyamg-5.2.1.tar.gz#sha256=f449d934224e503401ee72cd2eece1a29d893b7abe35f62a44d52ba831198efa
build_tools/azure/pylatest_pip_openblas_pandas_linux-64_conda.lock:# pip scikit-image @ https://files.pythonhosted.org/packages/5d/c5/bcd66bf5aae5587d3b4b69c74bee30889c46c9778e858942ce93a030e1f3/scikit_image-0.24.0.tar.gz#sha256=5d16efe95da8edbeb363e0c4157b99becbd650a60b77f6e3af5768b66cf007ab

I am having a quick look at pyamg Python 3.13 support in pyamg/pyamg#426.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants