Allow users to create table and image parameters from local data #1077

Fanjia-Yan · 2023-03-11T01:01:49Z

Describe your changes and why you are making these changes

This PR adds feature to use local data by creating a parameter data = client.create_param("data", default="path/to/data.csv", as_type=ArtifactType.TABLE, format="csv"). The created param will behave normally as a parameter artifact.

The current size of local data is set to have a maximum of 32MB. The reason for limiting the local data is because our server has a default limit for parsing http request.

Testing

Add 3 Test suite of integration tests:

Basic Functionality of using table local data: Publishing workflow using parameter from local csv, parquet, and json file
Basic Functionality of using image local data: Publishing workflow using parameter from local image
Error checking test: Check if invalid file path, mismatch artifact type can be detected

Existing Problem

Currently previewing and publishing workflow using local data parameter is extremely slow around (10 - 20 seconds) for a small(1 MB) of data. We have figured out that was due to serializing using json on Jupiter and json unmarshall the data on server.

Related issue number (if any)

https://linear.app/aqueducthq/issue/ENG-2584/allow-users-to-create-table-and-image-parameters-from-local-data

Loom demo (if any)

Checklist before requesting a review

I have created a descriptive PR title. The PR title should complete the sentence "This PR...".
I have performed a self-review of my code.
I have included a small demo of the changes. For the UI, this would be a screenshot or a Loom video.
If this is a new feature, I have added unit tests and integration tests.
I have run the integration tests locally and they are passing.
I have run the linter script locally (See python3 scripts/run_linters.py -h for usage).
All features on the UI continue to work correctly.
Added one of the following CI labels:
- run_integration_test: Runs integration tests
- skip_integration_test: Skips integration tests (Should be used when changes are ONLY documentation/UI)

kenxu95

Can you create a follow-up task for making this more performant and solving the sizing bug? 1MB is a pretty restrictive bar.

kenxu95 · 2023-03-13T21:53:00Z

.github/actions/setup-server/action.yml

@@ -38,6 +38,10 @@ runs:
      shell: bash
      run: pip3 install SQLAlchemy==1.4.46

+    - name: Install pyArrow for reading parquet table
+      shell: bash
+      run: pip3 install pyarrow==7.0.0


Shouldn't we add pyarrow to our SDK requirements.txt then? I'd rather we make our dependencies very explicit cc: @cw75

integration_tests/sdk/aqueduct_tests/param_test.py

sdk/aqueduct/utils/local_data.py

sdk/aqueduct/utils/serialization.py

sdk/aqueduct/artifacts/create.py

kenxu95 · 2023-03-21T20:22:11Z

@Fanjia-Yan just confirming that this is still being worked on and doesn't require a review from me yet

Fanjia-Yan · 2023-03-21T22:22:07Z

@Fanjia-Yan just confirming that this is still being worked on and doesn't require a review from me yet

There is one thing I would like to try before requesting a review. The File limit is currently set to 1MB which I am getting more and more disturbed by the value and I hope to get this thing straight before merging into main. I observe that the when we are parsing http request from server, large json file(>32MB), the parsing is faulty but no error is returned. This might be some issue of max file size we can set or work around in golang http request library. I am still investigating that.

…ub.com:aqueducthq/aqueduct into eng-2584-allow-users-to-create-table-and-image

sdk/aqueduct/backend/api_client.py

src/golang/cmd/server/request/payload.go

kenxu95

I think this is good! I do think this means we have add pyarrow as a requirement then to our SDK right? Want to get Chengang's confirmation on this, but could you add this requirement to our SDK? We're going to need to do it when we transition to parquet from json anyways.

sdk/aqueduct/backend/api_client.py

kenxu95 · 2023-03-23T22:45:33Z

sdk/aqueduct/backend/api_client.py

+            artifact_metadata.from_local_data for artifact_metadata in list(dag.artifacts.values())
+        ):
+            raise InvalidUserActionException(
+                "Local Data after serialization are too large. Aqueduct uses json serialization. The maximum size of workflow with local data is %sMB, the current size is %sMB."


nit: "Local data after serialization is too large".

kenxu95

It's very close, but not quite production quality yet. The big issue I have is with the overly aggressive restriction on pyarrow version. Let's always set this to the latest if we can get away with it.

kenxu95 · 2023-03-28T18:59:19Z

sdk/requirements/python-3-9.txt

@@ -7,6 +7,7 @@ IPython<=8.9.0
 croniter<=1.3.8
 pydantic>=1.9.0,<=1.10.4
 plotly<=5.13.0
+pyarrow <=7.0.0


Wait this is way too restrictive for users. The latest version of pyarrow is v11.0.0 right? Can we set that as the upper bound? (After you test it against v11, ofc :)

kenxu95 · 2023-03-28T19:00:03Z

sdk/aqueduct/client.py

            description:
                A description of what this parameter represents.
-
+            use_local:
+                whether this parameter uses local data source or not.


Please capitalize.

kenxu95 · 2023-03-28T19:00:51Z

sdk/aqueduct/client.py

+        default: Any,
+        description: str = "",
+        use_local: bool = False,
+        as_type: Optional[ArtifactType] = None,


Can we just call this as?

I think as is a python keyword, overriding it with a variable might be a bit tricky. Will something like _as or astype suffice?

ok, let's just keep it as_type then

sdk/aqueduct/client.py

kenxu95 · 2023-03-28T19:07:09Z

sdk/aqueduct/client.py

@@ -188,19 +196,39 @@ def create_param(self, name: str, default: Any, description: str = "") -> BaseAr
                The name to assign this parameter.
            default:
                The default value to give this parameter, if no value is provided.
-                Every parameter must have a default.
+                Every parameter must have a default. If decided to use local data,


grammer: "if we decide"

kenxu95 · 2023-03-28T19:08:34Z

sdk/aqueduct/client.py

+            use_local:
+                whether this parameter uses local data source or not.
+            as_type:
+                The expected type of the local data. Currently Local Data has support for ArtifactType.TABLE


Can we replace the second sentence with "Only supported types are ArtifactType.TABLE and ArtifactType.IMAGE

sdk/aqueduct/client.py

kenxu95 · 2023-03-28T19:09:47Z

sdk/aqueduct/client.py

        return create_param_artifact(
            self._dag,
            name,
            default,
            description,
            explicitly_named=True,
+            is_local_data=is_local_data,


Can't we just use use_local here?

kenxu95 · 2023-03-28T19:10:34Z

sdk/aqueduct/client.py

+                and ArtifactType.IMAGE.
+            format:
+                If local data type is ArtifactType.TABLE, the user has to specify the table format.
+                We currently support "JSON", "CSV", and "Parquet".


Let's keep the captialization consistent between the three examples: eg: "json", "csv", "parquet"

Fanjia-Yan · 2023-03-30T19:48:09Z

Fixed all the grammar and error message suggestion. The newest pyarrow 11.0.0 behaves correctly so I have set the requirement to that :)

kenxu95

Cool! lgtm just one super nit :)

kenxu95 · 2023-03-31T18:14:16Z

sdk/aqueduct/client.py

+        if use_local:
+            if not isinstance(default, str):
+                raise InvalidUserArgumentException(
+                    "the default value must be a path to local data."


super nit: Capitalize "the"

…llow-users-to-create-table-and-image

Fanjia Yan added 3 commits March 9, 2023 18:10

init commit

ac14072

read file directly instad of from bytes

5e9ee10

validate data and set maximum

d1d9576

Fanjia-Yan requested a review from kenxu95 March 11, 2023 01:22

Fanjia-Yan marked this pull request as ready for review March 11, 2023 01:44

Fanjia-Yan and others added 5 commits March 10, 2023 19:41

Merge branch 'main' into eng-2584-allow-users-to-create-table-and-image

e5a9aee

integration tests

53c91ec

lint

3aaf79a

merge

b728d1e

mypy

4083b4f

Fanjia-Yan added the run_integration_test Triggers integration tests label Mar 13, 2023

gh action

f8bef3e

kenxu95 requested changes Mar 13, 2023

View reviewed changes

Fanjia Yan added 7 commits March 14, 2023 11:19

naming convention

009f242

integration test fix

be9edb1

lint

6c4c4b6

fix

d6cd37e

complete hotel review file

90e7caf

aqueduct.LocalData()

32cc991

lint

2adbc7c

Fanjia Yan and others added 6 commits March 22, 2023 17:03

add request limitation comment

03f2085

Add use_local flag to client.publish_flow() (#1095)

df46796

Merge branch 'eng-2584-allow-users-to-create-table-and-image' of gith…

053f2af

…ub.com:aqueducthq/aqueduct into eng-2584-allow-users-to-create-table-and-image

Merge branch 'main' into eng-2584-allow-users-to-create-table-and-image

62d5f2c

change param test

fe5a06b

golang lint

7208b8e

Fanjia-Yan commented Mar 23, 2023

View reviewed changes

sdk/aqueduct/backend/api_client.py Show resolved Hide resolved

Fanjia-Yan commented Mar 23, 2023

View reviewed changes

src/golang/cmd/server/request/payload.go Show resolved Hide resolved

Fanjia-Yan requested a review from kenxu95 March 23, 2023 16:26

kenxu95 reviewed Mar 23, 2023

View reviewed changes

kenxu95 self-requested a review March 23, 2023 23:38

kenxu95 approved these changes Mar 23, 2023

View reviewed changes

Fanjia Yan and others added 10 commits March 24, 2023 10:11

retrive back integration test

bdd1c8e

lint

6d9e1ed

modify based on suggestion

bcd0916

Merge branch 'main' into eng-2584-allow-users-to-create-table-and-image

9708d0d

req

b2a6af7

refactor

912e021

fix mypy

8e048d3

test

3d9e64d

validate local data

2c62e26

change test

0cc7ee7

Fanjia-Yan requested a review from kenxu95 March 27, 2023 22:02

Fanjia Yan and others added 2 commits March 27, 2023 15:22

error message match

c2bacc1

regex match

ec2904d

kenxu95 requested changes Mar 28, 2023

View reviewed changes

Fanjia Yan and others added 2 commits March 29, 2023 13:11

pyarrow version change

34914dc

fix error message

2858320

Fanjia-Yan requested a review from kenxu95 March 30, 2023 19:47

kenxu95 approved these changes Mar 31, 2023

View reviewed changes

Fanjia Yan added 2 commits March 31, 2023 16:12

Merge branch 'main' of github.com:aqueducthq/aqueduct into eng-2584-a…

113f32e

…llow-users-to-create-table-and-image

error message change

9c61748

Fanjia-Yan merged commit d9d782e into main Apr 1, 2023

jpurusho65 pushed a commit that referenced this pull request Apr 3, 2023

Allow users to create table and image parameters from local data (#1077)

388eb9d

vsreekanti deleted the eng-2584-allow-users-to-create-table-and-image branch April 4, 2023 18:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow users to create table and image parameters from local data #1077

Allow users to create table and image parameters from local data #1077

Fanjia-Yan commented Mar 11, 2023 •

edited

Loading

kenxu95 left a comment

kenxu95 Mar 13, 2023

kenxu95 commented Mar 21, 2023

Fanjia-Yan commented Mar 21, 2023

kenxu95 left a comment

kenxu95 Mar 23, 2023

kenxu95 left a comment

kenxu95 Mar 28, 2023

kenxu95 Mar 28, 2023

kenxu95 Mar 28, 2023

Fanjia-Yan Mar 29, 2023

kenxu95 Mar 29, 2023

kenxu95 Mar 28, 2023

kenxu95 Mar 28, 2023

kenxu95 Mar 28, 2023

kenxu95 Mar 28, 2023

Fanjia-Yan commented Mar 30, 2023

kenxu95 left a comment

kenxu95 Mar 31, 2023

Allow users to create table and image parameters from local data #1077

Allow users to create table and image parameters from local data #1077

Conversation

Fanjia-Yan commented Mar 11, 2023 • edited Loading

Describe your changes and why you are making these changes

Testing

Existing Problem

Related issue number (if any)

Loom demo (if any)

Checklist before requesting a review

kenxu95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenxu95 commented Mar 21, 2023

Fanjia-Yan commented Mar 21, 2023

kenxu95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenxu95 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

Fanjia-Yan commented Mar 30, 2023

kenxu95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fanjia-Yan commented Mar 11, 2023 •

edited

Loading