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

Skip 2nd serialization pass of DeviceSerialized #309

Merged
merged 3 commits into from
Jun 29, 2020

Conversation

jakirkham
Copy link
Member

@jakirkham jakirkham commented Jun 5, 2020

Requires PR ( dask/distributed#3639 ) (so latest Dask release)
Finishes the work in PR ( #268 )

As "dask" serialization already converts a CUDA object into headers and frames that Dask is able to work with, drop code that tries to serialize frames on host further (as they are already as simple as they can be). Cuts a fair bit of boilerplate from the spilling path, which should simplify things a bit.

@jakirkham jakirkham force-pushed the drop_2nd_ser_pass branch 2 times, most recently from e3754eb to dc4f3c0 Compare June 5, 2020 19:36
As `"dask"` serialization already converts a CUDA object into headers
and frames that Dask is able to work with, drop code that tries to
serialize frames on host further (as they are already as simple as they
can be). Cuts a fair bit of boilerplate from the spilling path, which
should simplify things a bit.
@jakirkham jakirkham force-pushed the drop_2nd_ser_pass branch from dc4f3c0 to f36593e Compare June 5, 2020 20:20
@codecov-commenter
Copy link

codecov-commenter commented Jun 5, 2020

Codecov Report

Merging #309 into branch-0.15 will decrease coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.15     #309      +/-   ##
===============================================
- Coverage        59.29%   59.19%   -0.10%     
===============================================
  Files               16       16              
  Lines             1275     1272       -3     
===============================================
- Hits               756      753       -3     
  Misses             519      519              
Impacted Files Coverage Δ
dask_cuda/device_host_file.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d902ea...c7c6eea. Read the comment docs.

@jakirkham
Copy link
Member Author

So this works now. Though will wait until Distributed 2.18.0 is out so we can bump the dependency. Should be out later today or early next week. Leaving as draft/WIP until then.

@pentschev
Copy link
Member

Is 2.18.0 strictly necessary for this?

@jakirkham
Copy link
Member Author

Yeah we need the fix in PR ( dask/distributed#3639 ). This is what we determined when trying this previously ( #256 (comment) ).

@jakirkham jakirkham changed the title WIP: Skip 2nd serialization pass of DeviceSerialized Skip 2nd serialization pass of DeviceSerialized Jun 6, 2020
@jakirkham jakirkham marked this pull request as ready for review June 6, 2020 03:36
@jakirkham jakirkham requested review from a team as code owners June 6, 2020 03:36
@pentschev
Copy link
Member

Yeah we need the fix in PR ( dask/distributed#3639 ). This is what we determined when trying this previously ( #256 (comment) ).

Ok, makes sense then. Thanks @jakirkham !

@pentschev
Copy link
Member

@rapidsai/ops-codeowners could you approve this?

@jakirkham jakirkham marked this pull request as draft June 9, 2020 05:04
@jakirkham
Copy link
Member Author

Marked as draft as there are some orthogonal issues that were found with Dask + Distributed 2.18.0. So want to hold off on bumping the requirement/merging until we have addressed those.

@jakirkham
Copy link
Member Author

Marked as draft as there are some orthogonal issues that were found with Dask + Distributed 2.18.0. So want to hold off on bumping the requirement/merging until we have addressed those.

The underlying Distributed issue has been identified and filed as issue ( dask/distributed#3920 ). This is being addressed by PR ( dask/distributed#3922 ).

@jakirkham jakirkham marked this pull request as ready for review June 26, 2020 21:11
@jakirkham
Copy link
Member Author

Marking as ready for review as the orthogonal issues have been fixed upstream.

Copy link
Contributor

@dillon-cullinan dillon-cullinan left a comment

Choose a reason for hiding this comment

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

Does an update need to be made to integration for this? Current meta package is pinned with >=2.15.0

@jakirkham
Copy link
Member Author

Good point. Submitted an update in PR ( rapidsai/integration#64 ).

@quasiben
Copy link
Member

Thank you all! Merging in

@quasiben quasiben merged commit 30def7e into rapidsai:branch-0.15 Jun 29, 2020
@jakirkham jakirkham deleted the drop_2nd_ser_pass branch June 29, 2020 21:33
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.

5 participants