Skip to content

Conversation

@ayushdg
Copy link
Collaborator

@ayushdg ayushdg commented Feb 12, 2022

Removes the distributed.utils_test fixtures from conftest and creates a new fixture to generate the client.
The primary motivation for this is that the client fixture from distributed also resets all non dask configurations that impacts changes in #392

@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2022

Codecov Report

Merging #398 (a818de8) into main (07f1649) will decrease coverage by 6.38%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #398      +/-   ##
==========================================
- Coverage   95.48%   89.09%   -6.39%     
==========================================
  Files          68       68              
  Lines        2987     3337     +350     
  Branches      554      653      +99     
==========================================
+ Hits         2852     2973     +121     
- Misses         83      289     +206     
- Partials       52       75      +23     
Impacted Files Coverage Δ
dask_sql/_version.py 34.00% <0.00%> (-66.00%) ⬇️
dask_sql/__init__.py 100.00% <0.00%> (ø)
dask_sql/mappings.py 100.00% <0.00%> (ø)
dask_sql/physical/rex/core/call.py 99.12% <0.00%> (+<0.01%) ⬆️
dask_sql/physical/rel/custom/tables.py 85.71% <0.00%> (+2.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07f1649...a818de8. Read the comment docs.

@ayushdg
Copy link
Collaborator Author

ayushdg commented Feb 14, 2022

The dask cluster tests seem to be failing but I can't reproduce locally @charlesbluca any suggestions?

@ayushdg
Copy link
Collaborator Author

ayushdg commented Feb 18, 2022

Connected with @charlesbluca and thanks for the commit to fix the errors! Do you think it makes sense to merge this and open up an issue to discuss whether Dask-sql should indeed support running on a cluster which doesn't have dask-sql installed?

@ayushdg
Copy link
Collaborator Author

ayushdg commented Feb 22, 2022

@charlesbluca I've added a note around the fugue test that was disabled on an external cluster for the time being. Is this good to merge?

Copy link
Collaborator

@charlesbluca charlesbluca left a comment

Choose a reason for hiding this comment

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

Yup, I think this should be good to merge in - thanks for the work here @ayushdg 🙂

@charlesbluca charlesbluca merged commit 00df420 into dask-contrib:main Feb 23, 2022
@ayushdg ayushdg deleted the enh-pytest-client branch February 6, 2023 19:17
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