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

Client.upload_file send to both Workers and Scheduler #7802

Merged
merged 13 commits into from
May 9, 2023

Conversation

milesgranger
Copy link
Contributor

@milesgranger milesgranger commented Apr 26, 2023

Closes #7797

  • Tests added / passed

  • Passes pre-commit run --all-files

  • Moves the previous Worker.upload_file into Server.upload_file, giving access to both workers and the scheduler to a local_directory.

  • Impl of UploadFile for the Scheduler

  • Updates Client.upload_file to upload files to both scheduler and workers.

@milesgranger milesgranger marked this pull request as draft April 26, 2023 12:21
@github-actions
Copy link
Contributor

github-actions bot commented Apr 26, 2023

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       26 files  ±  0         26 suites  ±0   16h 7m 41s ⏱️ + 57m 33s
  3 559 tests +  5    3 449 ✔️ +  4     105 💤 ±0  5 +1 
45 041 runs  +63  42 910 ✔️ +61  2 126 💤 +1  5 +1 

For more details on these failures, see this check.

Results for commit 830f4e0. ± Comparison against base commit 57ae3e7.

♻️ This comment has been updated with latest results.

@milesgranger milesgranger force-pushed the 7797-file-upload-to-scheduler branch 2 times, most recently from c8718af to 337029a Compare April 27, 2023 08:17
@milesgranger milesgranger force-pushed the 7797-file-upload-to-scheduler branch from a559af1 to 9ec280e Compare April 27, 2023 12:30
@milesgranger milesgranger marked this pull request as ready for review April 27, 2023 13:14
@milesgranger
Copy link
Contributor Author

@jrbourbeau this is ready for review :)

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @milesgranger! This is looking good

The current main branch has upload_file related breakages, but no test coverage to account for those breakages. Is this something you could add here?

distributed/client.py Show resolved Hide resolved
distributed/tests/test_worker.py Show resolved Hide resolved
distributed/worker.py Outdated Show resolved Hide resolved
distributed/core.py Show resolved Hide resolved
distributed/core.py Outdated Show resolved Hide resolved
distributed/core.py Outdated Show resolved Hide resolved
distributed/core.py Outdated Show resolved Hide resolved
@milesgranger
Copy link
Contributor Author

When you have time @jrbourbeau I think this is ready for a final look.

@milesgranger
Copy link
Contributor Author

@jacobtomlinson I think James is out most of this week, would you be able to give this another look?

Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

This looks great thanks @milesgranger. It looks like you've addressed everything. If there is anything else from @jrbourbeau I'm sure we can address it in a follow up.

I might put out a warning tweet about renaming the temporary directly but other than that this should have minimal impact.

@jacobtomlinson jacobtomlinson merged commit 09ea0f5 into dask:main May 9, 2023
@jacobtomlinson
Copy link
Member

@milesgranger milesgranger deleted the 7797-file-upload-to-scheduler branch May 9, 2023 09:52
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.

File upload functionality can break with new pickle serialization changes
3 participants