Use orjson to serialize dict including np.array #8041

whitphx · 2024-04-16T18:22:24Z

Description

gr.JSON causes errors when the value includes NumPy arrays.
There are two different cases:

import gradio as gr
import numpy as np


def gendata():
    return np.zeros((10, 10))

demo = gr.Interface(
    fn=gendata,
    inputs=None,
    outputs="json",
)

demo.launch()

import gradio as gr
import numpy as np

with gr.Blocks() as demo:
    inp = gr.JSON(
        value=np.array([1, 2, 3])
    )
    out = gr.JSON()
    btn = gr.Button("Submit")
    btn.click(lambda x: x, inp, out)

demo.launch()

TODO:

Add tests -> Done

gradio-pr-bot · 2024-04-16T18:22:57Z

🪼 branch checks and previews

•	Name	Status	URL
	Spaces	ready!	Spaces preview
	Website	ready!	Website preview
	Storybook	ready!	Storybook preview
🦄	Changes	detected!	Details

Install Gradio from this PR

pip install https://gradio-builds.s3.amazonaws.com/4102b4bc0f1260dedcbbe6ec8d95666b7b615551/gradio-4.27.0-py3-none-any.whl

Install Gradio Python Client from this PR

pip install "gradio-client @ git+https://github.com/gradio-app/gradio@4102b4bc0f1260dedcbbe6ec8d95666b7b615551#subdirectory=client/python"

gradio-pr-bot · 2024-04-16T18:23:08Z

🦄 change detected

This Pull Request includes changes to the following packages.

Package	Version
`gradio`	`patch`

Maintainers can select this checkbox to manually select packages to update.

With the following changelog entry.

Use orjson to serialize dict including np.array

Maintainers or the PR author can modify the PR title to modify this entry.

Something isn't right?

Maintainers can change the version label to modify the version bump.
If the bot has failed to detect any changes, or if this pull request needs to update multiple packages to different versions or requires a more comprehensive changelog entry, maintainers can update the changelog file directly.

aliabid94 · 2024-04-16T19:28:12Z

Confirmed that this PR does not degrade performance

whitphx · 2024-04-16T19:37:39Z

Thank you! Let me add tests and some more fixes.

whitphx · 2024-04-16T21:24:02Z

When an error occurs at the /info endpoint on the server side, the HTTP error is caught here and no visual notification is displayed on the frontend.
I think the error notification to the user should be clearer. What do you think, and how can we do it here @pngwn @hannahblair ?

gradio/client/js/src/client.ts

Lines 317 to 321 in 5767310

    
           try { 
        
           	api = await view_api(config); 
        
           } catch (e) { 
        
           	console.error(`Could not get api details: ${e.message}`); 
        
           }

As an example, you can reproduce the error by running the code below on the main branch and it feels like the app crashes silently while we can see the error log on the server-side terminal.
The error itself will be fixed in this PR, but the error notification should be considered anyway?

import gradio as gr
import numpy as np

with gr.Blocks() as demo:
    inp = gr.JSON(
        value=np.array([1, 2, 3])
    )
    out = gr.JSON()
    btn = gr.Button("Submit")
    btn.click(lambda x: x, inp, out)

demo.launch()

freddyaboulton

Thanks for the PR @whitphx and nice that it does not degrade performance. Just left a few comments on the demos

freddyaboulton · 2024-04-17T18:36:10Z

js/app/test/json_component_interface.spec.ts

@@ -0,0 +1,21 @@
+import { test, expect } from "@gradio/tootils";


I don't think we need separate tests for Blocks and Interface since an Interface uses the same server as a Blocks

I added them because each caused a different error actually.

Interface: an error saying "Type is not JSON serializable: numpy.ndarray". This is fixed by https://github.com/gradio-app/gradio/pull/8041/files#diff-324a7165f5d5a8823a28b76f5653fa45f32c8144c82b2e528882c97c7eae534fR807-R812

Blocks: in this case, there is no visually observable error but the app silently crashes because of an error at the /info endpoint. This is fixed by https://github.com/gradio-app/gradio/pull/8041/files#diff-324a7165f5d5a8823a28b76f5653fa45f32c8144c82b2e528882c97c7eae534fR410

freddyaboulton · 2024-04-17T18:36:58Z

demo/json_component/run.py

@@ -1,6 +0,0 @@
-import gradio as gr 


This demo is used in the docs so we can't delete it

Oh missed it, sorry.
Revert renaming.

freddyaboulton · 2024-04-19T19:41:11Z

Actually I am doubting why we even need orjson at all?

The docstring of the json component's postprocess says Expects a strfilepath to a file containing valid JSON -- or alistordict that is valid JSON. Why would we expect passing a numpy array to work?

whitphx · 2024-04-19T20:12:32Z

Good point.
@aliabid94 and I discussed and said it's a "bug" atm, but it's also an option to leave this as-is as a design, as you wrote.
However, the following code works and it shows the input JSON data properly since the config including a NumPy array is serialized by orjson
so I think this inconsistency is confusing. This can be the answer to your comment (Why would we expect passing a numpy array to work?)

import gradio as gr
import numpy as np


demo = gr.Interface(
    fn=lambda x: x,
    inputs=gr.JSON(value=np.array([1, 2, 3])),
    outputs="json",
)

demo.launch()

If we treat this as a spec following the docstring, not a bug, the changes in this PR will be reverted.
Still, I think the problem of silent crash is a problem anyway.

freddyaboulton

Thanks for the clarification @whitphx @abidlabs . Let's use orjson for the serialization but I think we should update the docstring of the json component to say values that are orjson serializable are valid

freddyaboulton · 2024-04-19T21:29:18Z

js/app/test/json_component_blocks.spec.ts

@@ -0,0 +1,21 @@
+import { test, expect } from "@gradio/tootils";


The json_component_blocks demo does not exist so we should remove this test file.

Ah sorry forgot to rename this.
This should be json_component.spec.ts instead of deleting.

…component.spec.ts

abidlabs · 2024-04-21T06:01:51Z

demo/json_component_interface/run.py

Seems identical to the other demo, let's just delete this one?

#8041 (comment)

I think they are testing the same thing though. If we wanted to make sure the info route works, i think we can manually access that route with playwright?

abidlabs · 2024-04-21T06:06:09Z

We can change this to orjson.loads for faster performance:

gradio/gradio/components/json_component.py

Line 86 in 95c6bc8

return json.loads(value)

whitphx · 2024-04-22T14:20:43Z

I think we should update the docstring of the json component to say values that are orjson serializable are valid

I'm not sure if we should.
This orjson serializing is kind of a defensive fallback, rather than gr.JSON's spec.
In this sense, I can agree with your original opinion not to care about NumPy here as it's not gr.JSON's spec. However, the samples above were working with gradio<=4.22.0 but started to fail since 4.23.0 due to #7810 (comment) (sorry I didn't post this) so we can see it as a regression. This is my main reason to think we should use orjson here; because it's a regression, but not because it's a spec.

abidlabs · 2024-04-22T18:10:49Z

Good point. I'm leaning towards (2) because I think it is clearer what the postprocessing function for each component is doing. But what do you mean by this:

in this example, gr.Checkbox will have a "regression"

Why would there be a regression in gr.Checkbox if we switch gr.JSON to use orjson?

whitphx · 2024-04-23T05:26:30Z

Pushed a commit to take the option (2).

Why would there be a regression in gr.Checkbox if we switch gr.JSON to use orjson?

It was an example, and I wanted to say, we have to take care of this problem about all the components whose postprocess pass through the value without value conversion like the current gr.JSON and gr.Checkbox.
If we make only gr.JSON.postprocess use orjson, this problem is solved on it, but the same error still occurs with the sample code below. A NumPy array as gr.Checkbox's value doesn't make so much sense, but technically this example worked before 4.23.0, so it can be seen as a regression. (while we can see such an error for not sense-making values as a benefit.)

def gendata():
    return np.zeros((10, 10))

demo = gr.Interface(
    fn=gendata,
    inputs=None,
    outputs="checkbox",
)

demo.launch()

Anyway it's ok to patch each postprocess to make sure they never return non-JSON-serializable value no matter what value the user-defined fn returns.

In other words, without the globally-applied fallback like the option (1), we have to take care of the postprocess-s of all components.

whitphx · 2024-04-23T05:27:39Z

Added a patch to gr.Checkbox.postprocess to void this error for example.

whitphx · 2024-04-23T05:31:25Z

Found #7415 did a similar discussion.

freddyaboulton · 2024-04-23T15:36:21Z

Looks good @whitphx ! Thanks for making the fixes! Now that we do the json-handling in each component, we can use python unit tests as opposed to e2e tests?

abidlabs · 2024-04-23T16:25:23Z

gradio/components/checkbox.py

+        if isinstance(value, Iterable):
+            # Handles the cases of NumPy arrays for instance, in the same way as lists.
+            return len(value) > 0


I don't think we need this. It may technically be a breaking change to not include this, but the behavior when a numpy array is passed into a gr.Checkbox is very unclear and I think this addition will just confuse readers of the codebase.

In fact, when I test bool(np.array([])) it returns False with a warning that in future versions of numpy, it will return an error. So the breaking change is really on numpy's side. I think we can just return bool(value)

It may technically be a breaking change to not include this,

Exactly. This is what I meant as a "regression", and I added this commit as an example to clarify Checkbox's behavior in such edge cases.
The options are, (1) to cover as many cases as possible like this, or (2) to support only reasonable cases e.g. only bool value in the case of checkbox and raise an error for other values.
This commit is (1) to avoid the regression, and bool(value) as you wrote is (2). I think (2) is also a reasonable option.
Will take it.

Please note that bool(np.array([1,2])) raises a ValueError,
so if we take the option 2 (bool(value)), the app below still crashes without a clear error message. We will admit it as a design.

def gendata(): return np.zeros((10, 10)) demo = gr.Interface( fn=gendata, inputs=None, outputs="checkbox", ) demo.launch()

abidlabs · 2024-04-23T16:25:52Z

gradio/components/json_component.py

+            # Use orjson to convert NumPy arrays and datetime objects to JSON.
+            # This ensures a backward compatibility with the previous behavior.
+            # See https://github.com/gradio-app/gradio/pull/8041
+            return orjson.loads(


abidlabs · 2024-04-23T16:27:00Z

Looks good to me @whitphx. I agree with @freddyaboulton that it would be better if we could replace the e2e tests with python unit tests now for speed and remove the demo/json_component_interface/run.py as it should not be needed anymore

whitphx · 2024-04-24T09:20:35Z

gradio/components/json_component.py

@@ -83,9 +84,18 @@ def postprocess(self, value: dict | list | str | None) -> dict | list | None:
        if value is None:
            return None
        if isinstance(value, str):
-            return json.loads(value)
+            return orjson.loads(value)


@abidlabs @freddyaboulton Apart from this PR,
I found this implementation is different from the doc string which says

Expects a str filepath to a file containing valid JSON

while json.loads() expects a JSON string, not a file path (but json.load() does).

Which is the correct, the impl or the doc?

I think it's the implementation @whitphx !

Thanks, I updated the docstring about it together.

Nice catch @whitphx

…as they are not reasonable values to be interpreted as checkbox's value

whitphx · 2024-04-24T13:19:18Z

Updated.

freddyaboulton

Looks good to merge @whitphx ! Thanks for all the hard work on this

abidlabs · 2024-04-24T19:35:49Z

test/components/test_json_component.py

Nice to create this as a separate file. We should refactor test_components.py into separate component files to mirror the component .py files.

tbh I missed test_components.py 😅
Will do the refactoring in a separate PR.
Reminder -> #8125

abidlabs · 2024-04-24T19:42:12Z

gradio/components/json_component.py

@@ -38,7 +39,7 @@ def __init__(
    ):
        """
        Parameters:
-            value: Default value. If callable, the function will be called whenever the app loads to set the initial value of the component.
+            value: Default value as a valid JSON `str` -- or a `list` or `dict` that can be serialized to a JSON string. If callable, the function will be called whenever the app loads to set the initial value of the component.


I updated this as well.

abidlabs

Thanks for the fixes @whitphx!

whitphx requested a review from aliabid94 April 16, 2024 18:22

whitphx and others added 5 commits April 16, 2024 16:48

Use orjson to serialize dict including np.array

b447dc4

add changeset

74dca4a

Update json_component demo and add an E2E test using it

a80d1a7

Rename demo/json_component -> demo/json_component_blocks

eff616f

Add json_component_interface demo and an E2E test using it

d738dc0

whitphx force-pushed the json-component-numpy branch from 58a269a to d738dc0 Compare April 16, 2024 20:48

Fix to await assertion promises

880cf2c

whitphx requested a review from freddyaboulton April 16, 2024 21:47

whitphx marked this pull request as ready for review April 16, 2024 21:48

freddyaboulton reviewed Apr 17, 2024

View reviewed changes

Revert renaming of json_component demo

a4ac921

whitphx requested a review from freddyaboulton April 17, 2024 19:37

whitphx and others added 2 commits April 18, 2024 21:52

Merge branch 'main' into json-component-numpy

2afc788

Merge branch 'main' into json-component-numpy

56fd3e6

abidlabs added the v: patch A change that requires a patch release label Apr 18, 2024

add changeset

c1513f0

freddyaboulton approved these changes Apr 19, 2024

View reviewed changes

Rename js/app/test/json_component_blocks.spec.ts -> js/app/test/json_…

ede5103

…component.spec.ts

abidlabs reviewed Apr 21, 2024

View reviewed changes

Merge branch 'main' into json-component-numpy

bbb1bc1

whitphx added 2 commits April 23, 2024 05:29

Merge branch 'main' into json-component-numpy

b860b57

Revert changes in routes.py and add orjson to json_component.py

72cfe55

Update gr.Checkbox.postprocess to ensure a bool value is returned

26fb108

freddyaboulton added the t: fix A change that implements a fix label Apr 23, 2024

add changeset

f50e0bd

abidlabs reviewed Apr 23, 2024

View reviewed changes

whitphx commented Apr 24, 2024

View reviewed changes

whitphx added 4 commits April 24, 2024 18:22

Remove the if-block in gr.Checkbox.postprocess handling NumPy arrays …

6f1d0bc

…as they are not reasonable values to be interpreted as checkbox's value

Update gr.JSON's docstring

da3c934

Add test/components/test_json_component.py

0e6a542

Remove JSON component E2E tests

d98e8e8

whitphx mentioned this pull request Apr 24, 2024

Cache an error from app.submit() and show it on frontend #8115

Merged

Update gr.JSON's docstring

e9ada47

freddyaboulton approved these changes Apr 24, 2024

View reviewed changes

abidlabs reviewed Apr 24, 2024

View reviewed changes

docstring

4102b4b

abidlabs reviewed Apr 24, 2024

View reviewed changes

abidlabs approved these changes Apr 24, 2024

View reviewed changes

whitphx mentioned this pull request Apr 25, 2024

[Refactoring] Split the test cases in test_components.py into separate test files #8125

Closed

whitphx merged commit 937c858 into main Apr 25, 2024
7 checks passed

whitphx deleted the json-component-numpy branch April 25, 2024 07:13

whitphx mentioned this pull request May 6, 2024

Split test/test_components.py into test/components/test_*.py #8218

Merged

		@@ -0,0 +1,21 @@
		import { test, expect } from "@gradio/tootils";

Use orjson to serialize dict including np.array #8041

Use orjson to serialize dict including np.array #8041

Conversation

whitphx commented Apr 16, 2024 • edited

Description

gradio-pr-bot commented Apr 16, 2024 • edited

🪼 branch checks and previews

gradio-pr-bot commented Apr 16, 2024 • edited

🦄 change detected

This Pull Request includes changes to the following packages.

With the following changelog entry.

Something isn't right?

aliabid94 commented Apr 16, 2024

whitphx commented Apr 16, 2024

whitphx commented Apr 16, 2024

freddyaboulton 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

freddyaboulton commented Apr 19, 2024

whitphx commented Apr 19, 2024 • edited

freddyaboulton 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

abidlabs commented Apr 21, 2024

whitphx commented Apr 22, 2024

abidlabs commented Apr 22, 2024

whitphx commented Apr 23, 2024

whitphx commented Apr 23, 2024

whitphx commented Apr 23, 2024

freddyaboulton commented Apr 23, 2024

Choose a reason for hiding this comment

whitphx Apr 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abidlabs commented Apr 23, 2024

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

whitphx commented Apr 24, 2024

freddyaboulton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whitphx Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abidlabs left a comment

Choose a reason for hiding this comment

whitphx commented Apr 16, 2024 •

edited

gradio-pr-bot commented Apr 16, 2024 •

edited

gradio-pr-bot commented Apr 16, 2024 •

edited

whitphx commented Apr 19, 2024 •

edited

whitphx Apr 24, 2024 •

edited

whitphx Apr 25, 2024 •

edited