change json encoding ebhavior #1070

diptanu · 2024-11-28T05:35:47Z

The primary use case for JSON encoding is to support calling graphs with JSON data from other languages and retreive output from graphs as JSON. We were using JSON Pickle which adds a lot of un-necessary information in the json encoded result, and is confusing for the most part when deserializing in other language.

This PR -

Encodes the input and outputs as normal JSON. Any objects that can't be natively serialized with python's json module won't work. We would support native python types and dictionaries as inputs and outputs when JSON encoding is being used.
Introduce input and output encoders since in some cases we might want the input to be cloudpickle encoded for it's flexibility of serializing complex python types, but the output needs to be json encoded so that the output can be read back form typescript, Java or rust runtimes.

Test

Tested the simple function with json encoder graph.


(ve) diptanuc@Diptanus-MacBook-Pro-2 python-sdk % curl -X 'POST' \
  'http://localhost:8900/namespaces/default/compute_graphs/test_simple_function_with_json_encoding/invoke_object' \
  -H 'accept: */*' \
  -H 'Content-Type: application/json' \
  -d '"foo"'
data: {"id":"859fd5ec72107565"}

(ve) diptanuc@Diptanus-MacBook-Pro-2 python-sdk % curl -X 'GET' \
  'http://localhost:8900/namespaces/default/compute_graphs/test_simple_function_with_json_encoding/invocations/859fd5ec72107565/fn/simple_function_with_json_encoder/output/c894d638be414f01' \
  -H 'accept: */*'
"foob"%

diptanu · 2024-11-28T06:24:42Z

Tests needs to be fixed. Something broke after I removed a bunch of encoding related calls which were spread all over the code. We shouldn't be serializing and deserializing data outside of the object encoders

python-sdk/indexify/executor/agent.py

Default2882 · 2024-11-28T07:11:39Z

python-sdk/poetry.lock

@@ -954,6 +954,23 @@ files = [
    {file = "sniffio-1.3.1.tar.gz", hash = "sha256:f4324edc670a0f49750a81b895f35c3adb843cca46f0530f79fc1babb23789dc"},


We can do a poetry uninstall jsonpickle

python-sdk/tests/test_graph_behaviours.py

python-sdk/indexify/functions_sdk/indexify_functions.py

miguelhrocha

I'm going to approve to unblock myself, but I can tackle the feedback retroactively

seriousben

Left some questions and low priority proposed changes.

seriousben · 2024-12-03T15:38:23Z

python-sdk/indexify/executor/agent.py

-                    Text("registration Error: ", style="red bold")
-                    + Text(f"failed to register: {e}", style="red")
-                )
+                logging.error(f"failed to register: {e}")


Suggested change

logging.error(f"failed to register: {e}")

logging.error(f"failed to register:", exception=str(e))

seriousben · 2024-12-03T15:42:37Z

python-sdk/indexify/executor/downloader.py

+                "failed to download input",
+                url=url,
+                reducer_url=reducer_url,
+                error=response.text,


Suggested change

error=response.text,

error=response.text,

exception: str(e),

seriousben · 2024-12-03T15:42:53Z

python-sdk/indexify/executor/downloader.py

+                logger.error(
+                    "failed to download reducer output",
+                    url=reducer_url,
+                    error=response.text,


Suggested change

error=response.text,

error=response.text,

exception: str(e),

seriousben · 2024-12-03T15:43:30Z

python-sdk/indexify/executor/task_reporter.py

+                task_id=completed_task.task.id,
+                retries=completed_task.reporting_retries,
+                error=type(e).__name__,
+                message=str(e),


Suggested change

message=str(e),

exception=str(e),

seriousben · 2024-12-03T15:43:57Z

python-sdk/indexify/executor/task_reporter.py

+                task_id=completed_task.task.id,
+                retries=completed_task.reporting_retries,
+                status_code=response.status_code,
+                response_text=response.text,


Suggested change

response_text=response.text,

response_text=response.text,

exception: str(e),

seriousben · 2024-12-03T15:48:27Z

python-sdk/indexify/functions_sdk/object_serializer.py


    @staticmethod
    def serialize_list(data: List[Any]) -> str:
-        return jsonpickle.encode(data)
+        return json.dumps(data)


Should we wrap an exception into a value error here?

seriousben · 2024-12-03T15:48:34Z

python-sdk/indexify/functions_sdk/object_serializer.py


    @staticmethod
-    def deserialize_list(data: str) -> List[Any]:
-        return jsonpickle.decode(data)
+    def deserialize_list(data: str, t: Type) -> List[Any]:


Should we wrap an exception into a value error here?

seriousben · 2024-12-03T15:49:56Z

python-sdk/tests/test_graph_behaviours.py

+    val: int
+
+
+@indexify_function(accumulate=JsonSum, input_encoder="json")


Can we support accumulate=int?

@seriousben i think that will work too, because an int is serializable with json and cloudpickle

Default2882 reviewed Nov 28, 2024

View reviewed changes

seriousben reviewed Nov 28, 2024

View reviewed changes

python-sdk/tests/test_graph_behaviours.py Outdated Show resolved Hide resolved

seriousben reviewed Nov 28, 2024

View reviewed changes

python-sdk/indexify/functions_sdk/indexify_functions.py Outdated Show resolved Hide resolved

seriousben reviewed Nov 28, 2024

View reviewed changes

python-sdk/indexify/functions_sdk/indexify_functions.py Outdated Show resolved Hide resolved

diptanu added 8 commits December 2, 2024 15:33

change json encoding ebhavior

8ce2cd3

removing redundant serialization of outputs

1fb39d3

lint

5b0dbae

update json encoding behavior

e5063a8

removing jsonpickle from the project

40e1f61

update wip

461c3c8

fix tests

bb2f134

lint

b0ad30c

diptanu force-pushed the change-json-encoding-behavior branch from c7246ba to b0ad30c Compare December 2, 2024 23:35

diptanu added 2 commits December 2, 2024 22:54

using input and output encoders

f0a14e3

using default values for encoders

43e1294

miguelhrocha approved these changes Dec 3, 2024

View reviewed changes

seriousben approved these changes Dec 3, 2024

View reviewed changes

miguelhrocha merged commit 5776e0a into main Dec 3, 2024
5 checks passed

miguelhrocha deleted the change-json-encoding-behavior branch December 3, 2024 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change json encoding ebhavior #1070

change json encoding ebhavior #1070

diptanu commented Nov 28, 2024 •

edited

Loading

diptanu commented Nov 28, 2024

Default2882 Nov 28, 2024

miguelhrocha left a comment

seriousben left a comment

seriousben Dec 3, 2024

seriousben Dec 3, 2024

seriousben Dec 3, 2024

seriousben Dec 3, 2024

seriousben Dec 3, 2024

seriousben Dec 3, 2024

seriousben Dec 3, 2024

seriousben Dec 3, 2024

diptanu Dec 3, 2024

		@@ -954,6 +954,23 @@ files = [
		{file = "sniffio-1.3.1.tar.gz", hash = "sha256:f4324edc670a0f49750a81b895f35c3adb843cca46f0530f79fc1babb23789dc"},

	logging.error(f"failed to register: {e}")
	logging.error(f"failed to register:", exception=str(e))

	response_text=response.text,
	response_text=response.text,
	exception: str(e),

		val: int


		@indexify_function(accumulate=JsonSum, input_encoder="json")

change json encoding ebhavior #1070

change json encoding ebhavior #1070

Conversation

diptanu commented Nov 28, 2024 • edited Loading

Test

diptanu commented Nov 28, 2024

Choose a reason for hiding this comment

miguelhrocha left a comment

Choose a reason for hiding this comment

seriousben left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diptanu commented Nov 28, 2024 •

edited

Loading