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

Stricter Dask tests cleanup #1514

Merged
merged 2 commits into from
Oct 12, 2023
Merged

Conversation

ogrisel
Copy link
Contributor

@ogrisel ogrisel commented Oct 12, 2023

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

codecov bot commented Oct 12, 2023

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
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
14 of 16 checks passed
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant