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

Initial Python Interface for cufile Async IO #376

Merged
merged 10 commits into from
May 7, 2024
Merged

Initial Python Interface for cufile Async IO #376

merged 10 commits into from
May 7, 2024

Conversation

K-Wu
Copy link
Contributor

@K-Wu K-Wu commented May 5, 2024

Hi there,

Thanks for this great repository! I want to use the cuFile async IO in my research project and noticed this kvikio repo. However, the initial support has been done in #259 and tracked in #204, but the Python interface hasn't been done yet. So I exported the write_async and read_async to the CuFile Python class and added test case. This will be very helpful for my project where I want to do the PyTorch training computation and simultaneously load tensors from the SSDs. I created this PR because hopefully, it could be helpful for your repository as well as keeping the Python interface current.

Please let me know your thoughts. Thank you.

Best Regards,
Kun

@K-Wu K-Wu requested a review from a team as a code owner May 5, 2024 20:11
Copy link

copy-pr-bot bot commented May 5, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@K-Wu
Copy link
Contributor Author

K-Wu commented May 5, 2024

In this implementation, cuda stream's pointer is passed into the Python interface, which can be retrieved from the stream object in pycuda, cuda-python, torch. The stream objects in the three packages are interoperable as well. You may check the following gist on how to create a stream object based on another from another package and retrieve the address from these objects.

https://gist.github.com/K-Wu/178703e4803716d61c94df8938c3da9b

@madsbk madsbk added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels May 6, 2024
@madsbk
Copy link
Member

madsbk commented May 6, 2024

/ok to test

@K-Wu
Copy link
Contributor Author

K-Wu commented May 6, 2024

The failing tests in conda-python-tests are caused by torch being not installed. How shall I fix it? Maybe adding some flags to indicate this test requires Nvidia GPU and pytorch? Instead of pytorch, pycuda or cuda-python can also do the job.

@madsbk
Copy link
Member

madsbk commented May 6, 2024

The failing tests in conda-python-tests are caused by torch being not installed. How shall I fix it? Maybe adding some flags to indicate this test requires Nvidia GPU and pytorch? Instead of pytorch, pycuda or cuda-python can also do the job.

If we only need pytorch to create a new stream, I suggest that we use stream = cupy.cuda.Stream() and stream.ptr. KvikIO depend on cupy already.

Copy link
Member

@madsbk madsbk left a comment

Choose a reason for hiding this comment

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

Thanks @K-Wu, nice work!

@K-Wu
Copy link
Contributor Author

K-Wu commented May 6, 2024

@madsbk Hi Mads, thanks for this very quick response and detailed review! I will address your comments tonight.

Best Regards,
Kun

@K-Wu K-Wu requested a review from madsbk May 7, 2024 03:12
@madsbk
Copy link
Member

madsbk commented May 7, 2024

/ok to test

Copy link
Member

@madsbk madsbk left a comment

Choose a reason for hiding this comment

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

Looks good, I only have some minor suggestions

Comment on lines 37 to 38
with pytest.raises(RuntimeError, match="unsupported file open flags"):
f.raw_read_async(a, stream.ptr).check_bytes_done()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
with pytest.raises(RuntimeError, match="unsupported file open flags"):
f.raw_read_async(a, stream.ptr).check_bytes_done()
future_stream = f.raw_read_async(a, stream.ptr)
with pytest.raises(RuntimeError, match="unsupported file open flags"):
# The exception is raised when we check for completion.
future_stream.check_bytes_done()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comments addressed. However, the exception is actually raised when .raw_read_async was triggered. I added in the comment that this is the expected behavior.

@K-Wu
Copy link
Contributor Author

K-Wu commented May 7, 2024

Hi @madsbk, thanks for the review! Please let me know if further actions need to be done.

Also, shall I update the branch by clicking the "Update branch" button? It seems this repo used squash merge to deal with PRs so it seems okay. I know some people prefer rebase when pulling the main branch.

@K-Wu K-Wu requested a review from madsbk May 7, 2024 11:32
@madsbk
Copy link
Member

madsbk commented May 7, 2024

/ok to test

Copy link
Member

@madsbk madsbk left a comment

Choose a reason for hiding this comment

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

Nitpick

@madsbk
Copy link
Member

madsbk commented May 7, 2024

/ok to test

@K-Wu
Copy link
Contributor Author

K-Wu commented May 7, 2024

Done addressing the comments

@K-Wu K-Wu requested a review from madsbk May 7, 2024 12:14
@madsbk
Copy link
Member

madsbk commented May 7, 2024

/ok to test

@K-Wu
Copy link
Contributor Author

K-Wu commented May 7, 2024

@madsbk Hi Mads, thank you for your further reviews and edits!

@K-Wu K-Wu requested a review from madsbk May 7, 2024 13:15
@madsbk
Copy link
Member

madsbk commented May 7, 2024

Thanks @K-Wu

@madsbk
Copy link
Member

madsbk commented May 7, 2024

/merge

@rapids-bot rapids-bot bot merged commit 6ed7bcc into rapidsai:branch-24.06 May 7, 2024
35 checks passed
@K-Wu
Copy link
Contributor Author

K-Wu commented May 7, 2024

Thank you! @madsbk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants