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

Enable python-xdist on all tests #2059

Merged
merged 2 commits into from
Feb 29, 2024

Conversation

bmuskalla
Copy link
Contributor

@bmuskalla bmuskalla commented Feb 28, 2024

Enable python-xdist on all tests. This reduces overall test execution by 55% on CI down to a around 6min (from 12min).
After experimenting, using 4 instead of auto processes seems to avoid too much context trashing.

Total duration Comment
Baseline 12m 13s Workflow
xdist -n auto 6m 33s Workflow ubuntu a bit slower, windows a lot faster
xdist -n 4 5m 46s Workflow

Open questions:

  • The combinations that do work, can they be folded into a single test matrix configurarion (essentially whether there is still a need for Repository only/Everything else/torch; the others like lfs/fastai/tensorflow are still required as they need specific python versions. Even though this can be improved, it's likely not worth it as the windows configuration will be the dominating factor for overall CI time.
  • Do Windows tests pass at all through xdist?
  • Should the "Repository only" tests keep using -n 4 vs -n auto (rate limit concerns)

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Copy link

codecov bot commented Feb 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.70%. Comparing base (e7f243c) to head (d4dd11a).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2059   +/-   ##
=======================================
  Coverage   80.70%   80.70%           
=======================================
  Files          71       71           
  Lines        8519     8519           
=======================================
  Hits         6875     6875           
  Misses       1644     1644           

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

@bmuskalla bmuskalla marked this pull request as ready for review February 28, 2024 21:10
@Wauplin Wauplin mentioned this pull request Feb 29, 2024
4 tasks
@Wauplin
Copy link
Contributor

Wauplin commented Feb 29, 2024

Thanks for the wrap-up @bmuskalla! I still can't believe we were just one parameter away from enabling xdist in the CI without breaking any tests 😄

Regarding -n auto or -n 4, I believe it is exactly the same since github runners have 4 cores by default and pytest-xdist defaults to the number of cores. So the diff between the two runs is not really deterministic. Actually the biggest diff is due to the requirements install that took 1min less in one case -for no apparent reason-.

The combinations that do work, can they be folded into a single test matrix configurarion (essentially whether there is still a need for Repository only/Everything else/torch; the others like lfs/fastai/tensorflow are still required as they need specific python versions. Even though this can be improved, it's likely not worth it as the windows configuration will be the dominating factor for overall CI time.

Regarding this, the split is indeed for historical reasons when repository tests were taking much longer than the other tests. Here we could even divide Windows CI into 2 splits (repo and not repo) to reduce the overall CI but might be overkill. We can also reunify everything to reduce complexity. Let's keep those questions for later (I just opened #2062 not to forget) since this PR can be merged as-is and benefit everything right away!

Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

Thanks again for taking care of looking into this and benchmarking the improvement @bmuskalla! Let's get it merged! 🚀

@Wauplin Wauplin merged commit 92fc81a into huggingface:main Feb 29, 2024
14 checks passed
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