-
Notifications
You must be signed in to change notification settings - Fork 827
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
feat(dataflow): Use raw contents in joins if any message uses raw contents #4781
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
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.
A few suggestions on things that I think don't work currently, or which could be improved
scheduler/data-flow/src/main/kotlin/io/seldon/dataflow/kafka/StreamTransforms.kt
Outdated
Show resolved
Hide resolved
scheduler/data-flow/src/main/kotlin/io/seldon/dataflow/kafka/StreamTransforms.kt
Outdated
Show resolved
Hide resolved
scheduler/data-flow/src/main/kotlin/io/seldon/dataflow/kafka/protocol.kt
Outdated
Show resolved
Hide resolved
scheduler/data-flow/src/main/kotlin/io/seldon/dataflow/kafka/protocol.kt
Show resolved
Hide resolved
val v = input.contents.boolContentsList.flatMap { ByteBuffer.allocate(1) | ||
.order(ByteOrder.LITTLE_ENDIAN).put(if (it) {1} else {0}) .array().toList() }.toByteArray() | ||
} | ||
DataType.FP16, // Unclear if this is correct as will be stored as 4 byte floats |
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.
💭 It seems that Kotlin explicitly defines a Float
as having 4 bytes/32 bits. Given that, we could just allocate Float.SIZE_BYTES / 2
for the FP16
type.
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.
The issue is the source is a FP32 even if they type says FP16 so I think the best we can do now is leave as this or try to see how Triton store FP16 in their raw input contents
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.
In the interests of fixing the immediate issue, it's probably fine to leave for now, or to explicitly just drop the FP16 contents on the basis that wrong answers are worse than no answers.
Agree we should revisit for FP16, and (U)INT(8|16) as those probably suffer from the same issue.
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.
I think the uints won't as we allocate the correct size and push the correct value as an uint. I have updated fp16 to throw an exception.
scheduler/data-flow/src/main/kotlin/io/seldon/dataflow/kafka/protocol.kt
Outdated
Show resolved
Hide resolved
scheduler/data-flow/src/main/kotlin/io/seldon/dataflow/kafka/protocol.kt
Outdated
Show resolved
Hide resolved
scheduler/data-flow/src/main/kotlin/io/seldon/dataflow/kafka/protocol.kt
Outdated
Show resolved
Hide resolved
scheduler/data-flow/src/main/kotlin/io/seldon/dataflow/kafka/protocol.kt
Outdated
Show resolved
Hide resolved
scheduler/data-flow/src/main/kotlin/io/seldon/dataflow/kafka/protocol.kt
Outdated
Show resolved
Hide resolved
scheduler/data-flow/src/main/kotlin/io/seldon/dataflow/kafka/protocol.kt
Outdated
Show resolved
Hide resolved
scheduler/data-flow/src/main/kotlin/io/seldon/dataflow/kafka/protocol.kt
Outdated
Show resolved
Hide resolved
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.
Have discussed offline. We'll leave the exception handling for now and I'll aim to revisit that soon to ensure exceptions in the data handling don't cause the entire application to keel over.
In the meantime, approving.
We should probably add docs that:
- If any message in a join uses raw payloads, all messages will be converted.
- All compliant inference servers should support raw payloads. If users use one that doesn't but don't use any raw payloads with it, that'd be safe, but it's up to users to get right.
Triton will fail if some inputs have raw contents and some do not.
This PR ensures we always just have raw contents if one input does and another does not when joining streams of data.
Fixes: #4776