Skip to content

CI Use pytest-xdist in debian 32 build #32031

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Aug 29, 2025
Merged

CI Use pytest-xdist in debian 32 build #32031

merged 6 commits into from
Aug 29, 2025

Conversation

lesteve
Copy link
Member

@lesteve lesteve commented Aug 27, 2025 •
edited
Loading

Debian 32 build tends to be one of the slow build I think, something like ~25 minutes where others are more 10-20 minutes. Let's see if using pytest-xdist still has issues and whether it speeds things up a bit.

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 27, 2025 •
edited
Loading

✔️ Linting Passed

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

Generated for commit: 2121aee. Link to the linter CI: here

ogrisel
ogrisel reviewed Aug 28, 2025
View reviewed changes
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM. The last run was 16 min long. So there is definitely an improvement!

build_tools/azure/test_script.sh
@@ -58,8 +58,10 @@ if [[ "$COVERAGE" == "true" ]]; then
fi

if [[ "$PYTEST_XDIST_VERSION" != "none" ]]; then
XDIST_WORKERS=$(python -c "import joblib; print(joblib.cpu_count(only_physical_cores=True))")
TEST_CMD="$TEST_CMD -n$XDIST_WORKERS"
XDIST_WORKERS=$(python -c "import joblib; print(joblib.cpu_count())")
Copy link
Member

Choose a reason for hiding this comment

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

Why is this change needed? Is this caused by a bug or limitation in joblib?

Does this negatively impact other build times?

Copy link
Member Author

@lesteve lesteve Aug 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Basically right now, most (all?) of the Linux Azure VMs appears to have 1 physical cores and 2 logical cores 1, so we run pytest -n1 2. pytest -n2 speeds things up on debian-32 build but for some reason (maybe BLAS?) does not seem to have much effect on the other Linux builds, see #32031 (comment).

Footnotes

  1. this seems weird since those are vCPUs so I would expect the number of physical and logical to be the same 🤷? ↩

  2. which is a bit weird we may as well not use pytest-xdist in this case. This is another one of my unrelated change 😅. ↩

@lesteve
Copy link
Member Author

lesteve commented Aug 29, 2025 •
edited
Loading

It does speed-up the Debian 32 builds from ~25 minutes on main (see build logs log 1 log 2 log 3) to ~15 minutes (see build logs log 1 log 2 log 3).

3 CI runs on main are on the left, 3 on the right are from this PR (I wish there was a better way 😅):
image

The other builds don't seem affected or at least the difference is within the variability which is big sometimes. Note that using the number of logical cores does not affect OSX because OSX VMs seems to have num_logical_cores == num_physical_cores.

@lesteve lesteve marked this pull request as ready for review August 29, 2025 09:35
@ogrisel ogrisel merged commit 59c4b7a into scikit-learn:main Aug 29, 2025
36 checks passed
@lesteve lesteve deleted the debian-pytest-xdist branch August 29, 2025 15:37
@jeremiedbb jeremiedbb mentioned this pull request Sep 3, 2025
Open
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel approved these changes

Assignees
No one assigned
Labels
Build / CI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lesteve @ogrisel