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
Sync metadata with MLflow's model signature #285
Conversation
224854b
to
022d546
Compare
1f07a09
to
eeba837
Compare
eeba837
to
c79626e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
all minor comments really.
The only comment to double check is why to_metadata
is not being used apart from tests?
if common_length == -1: | ||
common_length = elem_length | ||
|
||
if common_length != elem_length: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how restrictive is it that we cannot pass different length strings at this stage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot. That is partly a limitation of the v2 protocol (and / or gRPC and protobuf). Since we pass everything as a tensor, we only have a flattened list of elements and a shape. Thus, is not possible to have variable lengths.
Having said that I do think that, at the moment, it will accept variable inputs if you use JSON (the codec will also accept inputs looking like ["asd", "hello world", "etc"]
). However, that won't work on the way back when we serialise the response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #164
Fixes #162
Changelog
binary
anddatetime
types)StringCodec