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

Update dask backend for compatibility with return_as=generator #1520

Merged
merged 7 commits into from Nov 13, 2023

Conversation

fcharras
Copy link
Contributor

@fcharras fcharras commented Nov 6, 2023

Reasonably simple changes, it seems to be just working as intended. Let's see if the CI agrees.

Are there particular tests to add on top ? the code paths for return_as=generator are the same than for return_as=list so test_dask already tests most of it.

Copy link

codecov bot commented Nov 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (05caf07) 94.99% compared to head (1f980ed) 95.06%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1520      +/-   ##
==========================================
+ Coverage   94.99%   95.06%   +0.07%     
==========================================
  Files          45       44       -1     
  Lines        7551     7564      +13     
==========================================
+ Hits         7173     7191      +18     
+ Misses        378      373       -5     
Files Coverage Δ
joblib/_dask.py 92.34% <100.00%> (-0.38%) ⬇️
joblib/_parallel_backends.py 93.91% <100.00%> (+0.12%) ⬆️
joblib/_utils.py 97.50% <100.00%> (+2.04%) ⬆️
joblib/test/test_parallel.py 96.66% <100.00%> (+0.48%) ⬆️

... and 6 files with indirect coverage changes

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

@fcharras fcharras marked this pull request as ready for review November 6, 2023 12:19
joblib/_dask.py Outdated Show resolved Hide resolved
@fcharras fcharras force-pushed the fea/dask_generator branch 7 times, most recently from b4957b2 to 25a6768 Compare November 10, 2023 10:33
@fcharras
Copy link
Contributor Author

fcharras commented Nov 10, 2023

@ogrisel the PR now proposes a fix to the coverage pipeline. The issue seems to have been that codecov and/or github integration do not support updating the github annotations dynamically with successive upload from parallel pipelines, but we had each pipeline individually upload their report. The first pipeline to reach codecov would trigger codecov annotations and those would not be cleared by subsequent updates, despite annotated code being actually covered by later pipelines. (I also suspect consecutive uploads do not merge correctly for python to begin with so this might have affected the final reports too ?)

I changed the script so that every pipeline creates a local artifact with its coverage report, and a single pipeline is triggered after all other are done that combines all coverage reports before issuing a single codecov upload.

That will also prevent receiving early e-mails from codecov because the codecov pipeline is temporarily failing because incomplete reports, despite later turning green with subsequent updates. It also fixes some unelegant codecov error messages in the pipelines.

While debugging I also found that the 3.8 distributed pipeline was not working because the pinned versions were uninstallable and caused pytest.importorskip("distributed") to just skip.

@fcharras fcharras force-pushed the fea/dask_generator branch 4 times, most recently from 83954fb to 4aead14 Compare November 10, 2023 13:02
@fcharras fcharras force-pushed the fea/dask_generator branch 3 times, most recently from ee7376e to cbc7604 Compare November 10, 2023 14:49
@fcharras
Copy link
Contributor Author

There were also issues when combining coverage reports from different pipelines because coverage reports stored absolute paths, and those diverged accross pipelines (macos / windows / linux absolute paths are different ?). Fixed now.

Copy link
Contributor

@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.

Thanks very much for the CI fix and the new feature.

Just a minor comment on naming, but otherwise LGTM!

joblib/_utils.py Outdated Show resolved Hide resolved
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.

None yet

2 participants