Correct integer widths in Rust/Python hand-off #7849
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Various locations in the Rust API take a `usize` as input. For the most part, Python integers will successfully be passed in at the correct size if they fit. Numpy `np.uint64` types, however, will fail conversion if running on a 32-bit machine. This is particularly relevant when the values are being passed as an array.
jakelishman
force-pushed
the
stochastic-swap-integer-sizes
branch
from
a5e970d
to
ef9beee
Compare
|
Yeah, we only need to worry about this when going from a numpy array to the rust extension as it's passed by reference from the underlying c array generated by numpy to the rust code (using numpy's c api). For the normal python -> rust path pyo3 will handle the conversion from an int type to the equivalent rust numeric type for us and if it's invalid (either because it's too large or negative for an unsigned int) it will raise an exception. |
jakelishman
force-pushed
the
stochastic-swap-integer-sizes
branch
from
ef9beee
to
7404e66
Compare
mtreinish
approved these changes
Mar 31, 2022
Pull Request Test Coverage Report for Build 2072577619
💛 - Coveralls |
mtreinish
added
the
Rust
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Various locations in the Rust API take a
usizeas input. For the mostpart, Python integers will successfully be passed in at the correct size
if they fit. Numpy
np.uint64types, however, will fail conversion ifrunning on a 32-bit machine. This is particularly relevant when the
values are being passed as an array.
Details and comments
Needs to test the wheel builds first - it's fine if it fails macos_arm because there's a separate fix in progress for that.
There are other places where we do an "arbitrary int" ->
usizehand-off, but I think that PyO3 does the right thing, even if given an effectiveu64to convert tou32, provided the represented value fits. I think this method is the only problematic one, because it won't convert arrays ofu64tou32(very reasonably).