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

CI/TST provide more informative traceback in case of a deadlock in tests #1379

Merged
merged 6 commits into from
Feb 17, 2023

Conversation

fcharras
Copy link
Contributor

@fcharras fcharras commented Feb 1, 2023

A set of changes proposed by @tomMoral in #588

@codecov
Copy link

codecov bot commented Feb 1, 2023

Codecov Report

Base: 94.01% // Head: 93.38% // Decreases project coverage by -0.64% ⚠️

Coverage data is based on head (8f06914) compared to base (6836640).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1379      +/-   ##
==========================================
- Coverage   94.01%   93.38%   -0.64%     
==========================================
  Files          52       52              
  Lines        7300     7342      +42     
==========================================
- Hits         6863     6856       -7     
- Misses        437      486      +49     
Impacted Files Coverage Δ
joblib/test/test_memmapping.py 96.99% <ø> (-2.26%) ⬇️
joblib/test/test_testing.py 100.00% <ø> (ø)
joblib/test/test_numpy_pickle.py 94.24% <100.00%> (+<0.01%) ⬆️
joblib/testing.py 95.23% <100.00%> (ø)
joblib/test/testutils.py 50.00% <0.00%> (-50.00%) ⬇️
joblib/backports.py 54.08% <0.00%> (-15.31%) ⬇️
joblib/_parallel_backends.py 92.27% <0.00%> (-2.21%) ⬇️
joblib/disk.py 90.47% <0.00%> (-1.59%) ⬇️
joblib/_memmapping_reducer.py 94.38% <0.00%> (-1.50%) ⬇️
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ogrisel
Copy link
Contributor

ogrisel commented Feb 15, 2023

There are plenty of unrelated changes in this PR. I will open a new PR with only the required change to fix the flake8 problem.

@ogrisel ogrisel changed the title Changes in testing framework and benchmark additions in preparation of return_generator work Make CI fail faster with more informative output in case of a deadlock in the tests Feb 15, 2023
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.

I changed the title to make it easy to understand the purpose of the PR without making any assumption about knowledge of concurrent PRs.

I think the new benchmark and the flake8 fix would have deserved independent PRs as those are unrelated and would have made it easier to review.

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

Actually I haven't reviewed the new benchmark.

Could you please rename
benchmarks/bench_njobs_1.py to benchmarks/bench_sequential_fast_tasks.py?

@glemaitre glemaitre self-requested a review February 17, 2023 10:00
@glemaitre glemaitre changed the title Make CI fail faster with more informative output in case of a deadlock in the tests CI/TST Make CI fail faster with more informative output in case of a deadlock in the tests Feb 17, 2023
@glemaitre glemaitre changed the title CI/TST Make CI fail faster with more informative output in case of a deadlock in the tests CI/TST provide more informative traceback in case of a deadlock in tests Feb 17, 2023
@ogrisel
Copy link
Contributor

ogrisel commented Feb 17, 2023

@tomMoral can you please push your updated version of the bench script to this PR to be able to merge it?

@glemaitre
Copy link
Member

@ogrisel we will remove the bench from this PR and added in a separate PR.

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.

+1

@ogrisel
Copy link
Contributor

ogrisel commented Feb 17, 2023

The memory.rst doctest failure is unrelated and is being addressed in #1396.

The other failure (non silent resource tracker) has also been observed elsewhere so is probably unrelated as well.

@ogrisel ogrisel merged commit 7bb4b94 into joblib:master Feb 17, 2023
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

3 participants