Skip to content
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

Remove msgpack from python-sdk and support application/json response from rust server #1025 and #1030 #1026

Merged
merged 35 commits into from
Nov 18, 2024

Conversation

Default2882
Copy link
Contributor

@Default2882 Default2882 commented Nov 13, 2024

Context

Completely removes dependency on msgpack in python-sdk, and now indexify supports both json and bytes encoding for input data. This change is for #1025 and #1030.

Testing

Added new graph behaviour test for graph with json as well as cloudpickle encoding.

Manual Testing -

  1. Invoking graph with json data where the start node has json encoding -
curl -H 'Content-Type: application/json' -d '{"x":"a","y":"10"}' -X POST http://localhost:8900/namespaces/default/compute_graphs/test_different_encoders/invoke_object
data: {"id":"dce70b20f0ab1a65"}
  1. Downloading output from a function with json encoding -
curl http://localhost:8900/namespaces/default/compute_graphs/test_different_encoders/invocations/db1919fac416a39a/fn/simple_function_multiple_inputs_json/output/70cd3c246dd5f2f9 -s | jq
"\"abbbbbbbbbb\""
  1. Downloading invoked payload of graph invocation where the start node has json encoding -
curl http://localhost:8900/namespaces/default/compute_graphs/test_different_encoders/invocations/db1919fac416a39a/payload -s | jq
{
  "x": "a",
  "y": 10
}

Contribution Checklist

  • [Done] If the python-sdk was changed, please run make fmt in python-sdk/.
  • [Done] If the server was changed, please run make fmt in server/.
  • [Done] Make sure all PR Checks are passing.

@Default2882 Default2882 changed the title [WIP] Remove msgpack from python-sdk, addresses #1025 Remove dependency on msgpack from python-sdk, addresses #1025 Nov 14, 2024
@Default2882 Default2882 marked this pull request as ready for review November 14, 2024 13:02
@diptanu
Copy link
Collaborator

diptanu commented Nov 15, 2024

@Default2882 This looks fine to me. Let's merge this once we have the other pieces for doing JSON end to end.

server/data_model/src/lib.rs Outdated Show resolved Hide resolved
server/src/routes/download.rs Outdated Show resolved Hide resolved
server/src/routes/download.rs Outdated Show resolved Hide resolved
server/src/routes/download.rs Outdated Show resolved Hide resolved
server/src/routes/download.rs Outdated Show resolved Hide resolved
server/src/routes/download.rs Outdated Show resolved Hide resolved
server/src/routes/download.rs Outdated Show resolved Hide resolved
…so that we don't store nested Indexify object.

Added 2 changes in task_reporter.py -
	- to include content_type in output of the function.
	- to only send the payload (json/bytes) to the server and not the whole IndexifyObject
@Default2882 Default2882 changed the title Remove dependency on msgpack from python-sdk, addresses #1025 Remove msgpack from python-sdk and support application/json response from rust server #1025 and #1030 Nov 16, 2024
@Default2882 Default2882 requested a review from diptanu November 17, 2024 04:01
@diptanu diptanu merged commit 53ed1f7 into tensorlakeai:main Nov 18, 2024
6 checks passed
@Default2882 Default2882 deleted the remove_msgpack branch November 18, 2024 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants