Fix Pandas codec decoding from numpy arrays#1751
Merged
Merged
Conversation
jesse-c
approved these changes
May 15, 2024
Contributor
|
We've gotten that fix in. @lhnwrk: could you please rebase this branch on top of |
Contributor
Author
|
@jesse-c Rebased on |
Contributor
|
@lhnwrk: Thank you! I've had to re-approve it and run the checks, so they're going now. As soon as it passes, I'll merge it in. |
Contributor
|
Merged in! Thank you again, @lhnwrk. 🙏🏻 |
This file contains hidden or 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
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.
Pandas codec currently breaks mlflow runtime's schema enforcement (#1625). This is because numeric mlflow datatypes are assigned
NumpyCodeccontent type by default, which decodesPandasCodec's explicit batch size into a column vector:However,
pd.Seriesexpects one-dimensional data, and when the_decoded_payloadis converted to apd.Serieshere returns unexpected results:This seems like a bug from pandas side,
dtype="int64"should have thrown an error (and in fact it does for higher dimension, but for some reason is okay with[np.array([1]), np.array([2]), np.array([3])]and weirdly casts them into tuples), but either way mlserver should have dropped the trailing axis aspd.Seriesalready implies column vector.From this test, it seems like the conversion to
listin the codec was to accommodate the use case ofpd.Seriesof tensors:I'd argue in the context of Pandas codec this should be made explicit with shape
[1, 1]and datatypeBYTESfor array elements. In fact, a test above does so, specifying the intended shape is a[2,1]column vector with datatypeBYTESforlistelements.This PR explicitly reshapes
np.arraypayload to expected shapes before conversion topd.Series.