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

Implement to_device() for python bindings, add unit tests #1611

Merged
merged 6 commits into from Feb 8, 2024

Conversation

ozancaglayan
Copy link
Contributor

@ozancaglayan ozancaglayan commented Feb 1, 2024

This will fix the issue #1575

  • Expose ctranslate2.Device enum to Python
  • Implement to_device() method that wraps the overloaded StorageView::to() variant
  • Write unit tests

Caveats:

  • Using a CPU StorageView and calling .to_device(Device.cpu) seems to create a copy of the backing store. I added an if () on the C++ side but didn't seem to work. I'm not super familiar with C++ and pybind so maybe there's a way to just reflect back the tensor/storage itself in that case.
  • When copying a CUDA Storage to CPU, would the original CUDA storage be garbage collected, cleaned up if it gets out of scope or would that risk leaking CUDA memory?

@ozancaglayan ozancaglayan changed the title Implement to_device() for python bindings, add unit tests (#1575) Implement to_device() for python bindings, add unit tests Feb 1, 2024
@ozancaglayan
Copy link
Contributor Author

ozancaglayan commented Feb 1, 2024

I don't know if the failure to build win wheels is related or not. Logs does not seem to provide anything useful.

OK found it: pypa/cibuildwheel#1741

@ozancaglayan
Copy link
Contributor Author

Hi @minhthuc2502 , would it be possible to review this?

src/storage_view.cc Outdated Show resolved Hide resolved
@minhthuc2502
Copy link
Collaborator

minhthuc2502 commented Feb 7, 2024

Could you fix the pipeline please?

@ozancaglayan
Copy link
Contributor Author

Yes, I'll look at it now

@ozancaglayan
Copy link
Contributor Author

I just merged master in, I can't see any particular reason other than a timeout in the previous pipeline run. Let's see what happens now.

@ozancaglayan
Copy link
Contributor Author

ok apparently this was a glitch in the build process rather than an issue in the PR itself.

@minhthuc2502
Copy link
Collaborator

minhthuc2502 commented Feb 8, 2024

LGTM! Thanks for your PR!

@minhthuc2502 minhthuc2502 merged commit 4c7b956 into OpenNMT:master Feb 8, 2024
17 checks passed
@ozancaglayan ozancaglayan deleted the py-add-to-device-method branch February 8, 2024 11:17
@ozancaglayan
Copy link
Contributor Author

Thanks! Do you have any plans of doing a release soon?

@minhthuc2502
Copy link
Collaborator

We will do the release ASAP

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

2 participants