Skip to content

Stricter Dask tests cleanup#1514

Merged
ogrisel merged 2 commits intojoblib:masterfrom
ogrisel:dask-stricter-test-cleanups
Oct 12, 2023
Merged

Stricter Dask tests cleanup#1514
ogrisel merged 2 commits intojoblib:masterfrom
ogrisel:dask-stricter-test-cleanups

Conversation

@ogrisel
Copy link
Copy Markdown
Contributor

@ogrisel ogrisel commented Oct 12, 2023 •
edited
Loading

On my laptop, running the latest version of dask and distributed, with tests that run in a random order for some reason,

  • the cleanup fixture (dependency of the loop fixture) would be not found by pytest (see Can't find cleanup pytest fixture dask/dask#9137),
  • tests executed after test_joblib_warning_inside_dask_daemonic_worker would either fail or error at teardown because of non-collected subprocess workers from that test.

@ogrisel
Copy link
Copy Markdown
Contributor Author

ogrisel commented Oct 12, 2023

Hopefully this will fix errors such as:

________________________ ERROR at setup of test_simple _________________________
file /home/runner/work/cloudpickle/joblib/joblib/test/test_dask.py, line 42
  def test_simple(loop):
file /opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/distributed/utils_test.py, line 127
  @pytest.fixture
  def loop(loop_in_thread):
file /opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/distributed/utils_test.py, line 132
  @pytest.fixture
  def loop_in_thread(cleanup):
E       fixture 'cleanup' not found
>       available fixtures: cache, capfd, capfdbinary, caplog, capsys, capsysbinary, doctest_namespace, loop, loop_in_thread, monkeypatch, pytestconfig, record_property, record_testsuite_property, record_xml_attribute, recwarn, tmp_path, tmp_path_factory, tmpdir, tmpdir_factory
>       use 'pytest --fixtures [testpath]' for help on them.

observed in:

https://github.com/cloudpipe/cloudpickle/actions/runs/6485430661/job/17611486887?pr=519

from cloudpipe/cloudpickle#519

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 12, 2023 •
edited
Loading

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (2ce6282) 95.00% compared to head (b8bbb3c) 95.02%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1514      +/-   ##
==========================================
+ Coverage   95.00%   95.02%   +0.01%     
==========================================
  Files          45       45              
  Lines        7550     7553       +3     
==========================================
+ Hits         7173     7177       +4     
+ Misses        377      376       -1
Files Coverage Δ
joblib/test/test_dask.py 92.33% <100.00%> (+1.13%) ⬆️

... and 5 files with indirect coverage changes

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

@ogrisel
Copy link
Copy Markdown
Contributor Author

ogrisel commented Oct 12, 2023

The 2 distributed runs pass. I will merge without waiting for the scikit-learn tests to be able to iterate faster on the cloudpickle CI.

@ogrisel ogrisel merged commit 6d40201 into joblib:master Oct 12, 2023
@ogrisel ogrisel deleted the dask-stricter-test-cleanups branch October 12, 2023 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@ogrisel