-
Notifications
You must be signed in to change notification settings - Fork 65
Cbor2 serialization for function calling [SDK-584] #3474
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
base: main
Are you sure you want to change the base?
Conversation
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.
Look good to me! Just minor comments on implementation, though also pending the larger discussion we started yesterday I guess.
async def package_output( | ||
item: Any, function_input: api_pb2.FunctionInput, input_id: str, retry_count: int | ||
) -> api_pb2.FunctionPutOutputsItem: | ||
assert function_input.data_format # TODO: Remove this - just for debugging |
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.
Debugging leftovers?
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.
Yup
Also refactors some of the conftest setup for unit tests, reducing duplication
e08c617
to
4a6fe91
Compare
…2-function-calling
@@ -2530,6 +2559,111 @@ def test_deserialization_error_returns_exception(servicer, client): | |||
assert int(deserialize(ret.items[1].result.data, ret.client)) == 4 | |||
|
|||
|
|||
@skip_github_non_linux | |||
def test_cbor_input_payload_simple_function(servicer): |
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.
TODO @freider : Add test for cbor generators
…2-function-calling
…2-function-calling
Adds opt-in support for cbor as a payload serialization format instead of pickle.
cbor2
to be installed in the container environment to work. This happens automatically for image builder 2025.06 and newer (if users absolutely need to use older image builder they can manually add cbor2 as a dependency to their image to make this work)_experimental_restrict_output=True
for now). This makes output cbor regardless if the input is pickle (good for security in the receiver, or if you know the consumer will be on libmodal even if the input creator is in Python using .spawn)TODO
supported_input_formats
in the client by sending cbor if pickle isn't supported by the Function definition. There is currently no syntactic way to restrict the input to a function when deploying it, but if we were to add this in the future it's good if the client respects it already for forwards-compatibility._experimental_restrict_output
to either the@app.cls
or@modal.method
decorators + add test that it's respectedNot in this patch (yet?):
exception
string repr of errors anyways at the moment. Will not do for this initial version - opting to leave these fields empty in the response payload for cbor inputs.Compatibility checklist
Check these boxes or delete any item (or this section) if not relevant for this PR.
Note on protobuf: protobuf message changes in one place may have impact to
multiple entities (client, server, worker, database). See points above.
Changelog