-
Notifications
You must be signed in to change notification settings - Fork 3
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
Adding implementation for PJRT_Executable_OutputElementTypes and PJRT_Executable_OutputDimensions #238
base: main
Are you sure you want to change the base?
Conversation
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #238 +/- ##
==========================================
- Coverage 77.30% 76.92% -0.39%
==========================================
Files 21 21
Lines 1018 1053 +35
==========================================
+ Hits 787 810 +23
- Misses 231 243 +12 ☔ View full report in Codecov by Sentry. |
ada4db8
to
0c8925c
Compare
private: | ||
// Checks whether m_output_dim_sizes and m_output_dims_concatenated have been |
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.
Describe this in a bit more detail, something like: "Check whether the information on output dimension has been populated".
Ticket
#164
Problem description
PJRT_Executable_OutputElementTypes
andPJRT_Executable_OutputDimensions
were unimplemented.What's changed
In
LoadedExecutableInstance::Execute
we got the shape for everyPJRT_Buffer
output frombinary.getProgramOutputs
andimage_->isOutputScalar
. Now we store the shape inExecutableImage
class and have a getter for the final shape, and notisOutputScalar
.Adding a vector that stores
PJRT_Buffer_Type
for every output inExecutableImage
, as well as a getter, for the implementation ofPJRT_Executable_OutputElementTypes
.PJRT_Executable_OutputDimensions
requires the dimensions to be in a different format. It returns an array of output dimension sizes and an array of concatenated dimensions, so we also store that type of pointer inExecutableImage
. We store it because we are responsible for memory deallocation, and not the caller ofPJRT_Executable_OutputDimensions
. But because concatenated dimensions are rarely needed, these fields arenullptr
untilget_output_dims_concatenated
is called.Notes
It is possible that
PJRT_Executable_OutputElementTypes
should return only different unique PJRT_Buffer_Types, and not a type for every output.We might want to store information about stride of every output in
ExecutableImage
as well, sobinary.getProgramOutput
wouldn't have to be called inLoadedExecutableInstance::Execute
, and all the information about outputs would be at one place.Checklist