Skip to content

Resolve ML cluster failures #957

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

Merged
merged 6 commits into from
Dec 6, 2022

Conversation

charlesbluca
Copy link
Collaborator

It looks like something changed in upstream Dask such that we're now seeing issues when trying to use the local ML wrapper functions introduced in #832 and #855 in an independent cluster; this PR wraps these relevant function usages with make_pickable_without_dask_sql as is done with other functions passed directly off to Dask, and adds the skip_if_external_scheduler marker to relevant tests that would otherwise need scikit-learn.

cc @sarahyurick

@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2022

Codecov Report

Merging #957 (8b75dd7) into main (f3fbdd0) will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #957      +/-   ##
==========================================
+ Coverage   76.62%   76.76%   +0.14%     
==========================================
  Files          73       73              
  Lines        4115     4119       +4     
  Branches      751      751              
==========================================
+ Hits         3153     3162       +9     
+ Misses        792      783       -9     
- Partials      170      174       +4     
Impacted Files Coverage Δ
dask_sql/physical/rel/custom/wrappers.py 64.59% <100.00%> (+0.52%) ⬆️
dask_sql/_version.py 32.76% <0.00%> (+1.41%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sarahyurick
Copy link
Collaborator

Interesting, I wasn't familiar with make_pickable_without_dask_sql but makes sense to me! Thanks

Copy link
Collaborator

@ayushdg ayushdg left a comment

Choose a reason for hiding this comment

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

changes lgtm! Thanks

@ayushdg ayushdg merged commit f26575b into dask-contrib:main Dec 6, 2022
@charlesbluca charlesbluca deleted the resolve-upstream-dask branch July 27, 2023 21:43
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.

4 participants