feat: add generic protobuf #2634
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
aarnphm
reviewed
Jun 24, 2022
aarnphm
reviewed
Jun 24, 2022
aarnphm
reviewed
Jun 24, 2022
aarnphm
approved these changes
Jun 24, 2022
aarnphm
reviewed
Jun 25, 2022
protos/io_descriptors.proto
Outdated
| * The repeated field for the identified dtype contains the array. | ||
| * Naming convention: Follows `numpy.dtype` where appropriate. | ||
| * `numpy.dtype`: https://numpy.org/doc/stable/reference/arrays.dtypes.html#specifying-and-constructing-data-types */ | ||
| message Array { |
There was a problem hiding this comment.
Suggested change
| message Array { | |
| message NumpyNdarray { |
Also i think the filename should be protos/io_descriptors/numpy.proto
There was a problem hiding this comment.
For the message name, I thought we were keeping it Array to prevent confusion from clients using languages other than python. If I separate all the io descriptors in their own proto, there will be one generated file for each proto. Having one io_descriptor proto means users can access them from one generated file. For example: io_descriptor_pb.Array() to create a new array and io_descriptor_pb.PandasDataframe() to create a pandas dataframe and so on.
There was a problem hiding this comment.
I think NumpyNdarray provides a better distinction than normal Array. Since a lot of the proto message is Numpy specific, thus the name NumpyNdarray makes it more clear than just Array.
There was a problem hiding this comment.
We don't do this in JSON, and I'm not sure we want to do this here. Numpy is what's being used in the server under the hood to process generic array data coming from clients, I don't think clients should care what the input format is.
There was a problem hiding this comment.
I made the change to NumpyNdarray. If you guys want, I can change it back to Array
There was a problem hiding this comment.
@sauyon imo we should follow the naming in io_descriptors for consistency
sauyon
reviewed
Jun 28, 2022
Lets merge this sauyon |
aarnphm
pushed a commit
that referenced
this pull request
Jun 29, 2022
* Added generic proto file with basic Value message * Added basic Array message to represent arrays * Added Tuple message to represent tuples * Added timestamp and duration datatypes * Added concise todo comments * Changed field names, message name `NumpyNdarray` * Applied changed message name, moved todo comment * Added trailing newline * Fixed sint32 and sint64 field names Co-authored-by: Sadab Hafiz <sadabhafizny@gmail.com> Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
aarnphm
pushed a commit
that referenced
this pull request
Jun 29, 2022
* Added generic proto file with basic Value message * Added basic Array message to represent arrays * Added Tuple message to represent tuples * Added timestamp and duration datatypes * Added concise todo comments * Changed field names, message name `NumpyNdarray` * Applied changed message name, moved todo comment * Added trailing newline * Fixed sint32 and sint64 field names Co-authored-by: Sadab Hafiz <sadabhafizny@gmail.com> Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
aarnphm
pushed a commit
that referenced
this pull request
Jun 29, 2022
* Added generic proto file with basic Value message * Added basic Array message to represent arrays * Added Tuple message to represent tuples * Added timestamp and duration datatypes * Added concise todo comments * Changed field names, message name `NumpyNdarray` * Applied changed message name, moved todo comment * Added trailing newline * Fixed sint32 and sint64 field names Co-authored-by: Sadab Hafiz <sadabhafizny@gmail.com> Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
aarnphm
pushed a commit
that referenced
this pull request
Jun 29, 2022
* Added generic proto file with basic Value message * Added basic Array message to represent arrays * Added Tuple message to represent tuples * Added timestamp and duration datatypes * Added concise todo comments * Changed field names, message name `NumpyNdarray` * Applied changed message name, moved todo comment * Added trailing newline * Fixed sint32 and sint64 field names Co-authored-by: Sadab Hafiz <sadabhafizny@gmail.com> Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
aarnphm
pushed a commit
that referenced
this pull request
Jul 7, 2022
* Added generic proto file with basic Value message * Added basic Array message to represent arrays * Added Tuple message to represent tuples * Added timestamp and duration datatypes * Added concise todo comments * Changed field names, message name `NumpyNdarray` * Applied changed message name, moved todo comment * Added trailing newline * Fixed sint32 and sint64 field names Co-authored-by: Sadab Hafiz <sadabhafizny@gmail.com> Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
aarnphm
pushed a commit
that referenced
this pull request
Jul 12, 2022
* Added generic proto file with basic Value message * Added basic Array message to represent arrays * Added Tuple message to represent tuples * Added timestamp and duration datatypes * Added concise todo comments * Changed field names, message name `NumpyNdarray` * Applied changed message name, moved todo comment * Added trailing newline * Fixed sint32 and sint64 field names Co-authored-by: Sadab Hafiz <sadabhafizny@gmail.com> Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
aarnphm
pushed a commit
that referenced
this pull request
Jul 14, 2022
* Added generic proto file with basic Value message * Added basic Array message to represent arrays * Added Tuple message to represent tuples * Added timestamp and duration datatypes * Added concise todo comments * Changed field names, message name `NumpyNdarray` * Applied changed message name, moved todo comment * Added trailing newline * Fixed sint32 and sint64 field names Co-authored-by: Sadab Hafiz <sadabhafizny@gmail.com> Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
aarnphm
pushed a commit
that referenced
this pull request
Jul 17, 2022
* Added generic proto file with basic Value message * Added basic Array message to represent arrays * Added Tuple message to represent tuples * Added timestamp and duration datatypes * Added concise todo comments * Changed field names, message name `NumpyNdarray` * Applied changed message name, moved todo comment * Added trailing newline * Fixed sint32 and sint64 field names Co-authored-by: Sadab Hafiz <sadabhafizny@gmail.com> Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
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.
Description
Added a generic protobuf file to represent arrays in protobuf format for grpc. Currently contains message
Valueto represent single instance of supported types, messageArrayto represent arrays of supported types, and messageNumpyNdarraywhich stores more details about the arrays similar to python numpy arrays.Motivation and Context
This is a step towards allowing serving through grpc.
How Has This Been Tested?
This has been tested by creating protobuf instances of different types of arrays using the generated code of this proto file.
Checklist:
make formatandmake lintscript have passed(instructions).