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.

Sign up for GitHub

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

Jump to bottom

Remove some old deprecated code #6144

Merged
merged 2 commits into from
Apr 19, 2022
Merged

Remove some old deprecated code #6144

merged 2 commits into from
Apr 19, 2022

Conversation

mrocklin
Copy link
Member

No description provided.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 16, 2022 •
edited
Loading

Unit Test Results

       16 files  +       6         16 suites  +6   7h 12m 38s ⏱️ + 3h 14m 29s
  2 727 tests  -        6    2 647 ✔️ +       4       80 💤  -   10  0 ❌ ±0 
21 701 runs  +8 408  20 666 ✔️ +7 942  1 035 💤 +466  0 ❌ ±0 

Results for commit 38b8d41. ± Comparison against base commit a179444.

♻️ This comment has been updated with latest results.

sjperkins
sjperkins reviewed Apr 18, 2022
View reviewed changes
Copy link
Member

@sjperkins sjperkins left a comment

Choose a reason for hiding this comment

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

These deprecations seem straightforward to me. It looks like test cases fail due to leaked semaphores on Windows py3.8 and a leaked thread on MacOS py3.8. Do you think this is a matter for concern?

@mrocklin
Copy link
Member Author

Definitely a concern. Our CI system needs love. I'm working on these issues separately in other PRs. I don't think that they're related to these changes.

@mrocklin
Copy link
Member Author

#6146 for the Thread leak. I'll probably leave the semaphore stuff for @fjetter (he's owned that feature historically)

@mrocklin mrocklin merged commit c9dcbe7 into dask:main Apr 19, 2022
@fjetter
Copy link
Member

fjetter commented Apr 19, 2022

I think the leaked semaphores are not distributed.Semaphores but multiprocessing Semaphores, aren't they?

@mrocklin mrocklin deleted the deprecated branch April 19, 2022 17:59
@mrocklin
Copy link
Member Author

See https://github.com/dask/distributed/runs/6048484364?check_suite_focus=true

The semaphore code in Dask comes up an increasing fraction of the time. This part of the codebase has tests that I don't understand well enough to operate in. As a result I'm not able to clean them up the way I am with the other parts of the codebase.

asford added a commit to asford/distributed that referenced this pull request May 11, 2022
@asford
Add idempotent to register_scheduler_plugin client
36aa7f3 
Updates client-side interface of register_scheduler_plugin to pass through idempotent flag.

Extending dask#5545

Finalizing removal of kwargs support
deprecated in dask#5699
and removed in dask#6144
This was referenced May 11, 2022
Closed
Merged
mrocklin pushed a commit that referenced this pull request May 13, 2022
@asford
Add idempotent to register_scheduler_plugin client (#6328)
bab29c5 
Updates client-side interface of register_scheduler_plugin to pass through idempotent flag.

Extending #5545

Finalizing removal of kwargs support
deprecated in #5699
and removed in #6144
@hendrikmakait hendrikmakait mentioned this pull request Jul 14, 2023
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjperkins sjperkins sjperkins approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mrocklin @fjetter @sjperkins