Allow users to create all types of Artifact from Local Data #1153
Conversation
…ub.com:aqueducthq/aqueduct into eng-2584-allow-users-to-create-table-and-image
…ub.com:aqueducthq/aqueduct into eng-2585-allow-users-to-create-all-types-of
…ub.com:aqueducthq/aqueduct into eng-2585-allow-users-to-create-all-types-of
…ub.com:aqueducthq/aqueduct into eng-2585-allow-users-to-create-all-types-of
Fanjia-Yan
changed the base branch from
eng-2584-allow-users-to-create-table-and-image
to
main
Fanjia-Yan
changed the base branch from
main
to
eng-2584-allow-users-to-create-table-and-image
Fanjia-Yan
changed the base branch from
eng-2584-allow-users-to-create-table-and-image
to
main
| return input | ||
|
|
||
| with open("data/test.pickle", "wb") as file: | ||
| pickle.dump(ArtifactType, file) |
There was a problem hiding this comment.
To test pickle local data. I save class ArtifactType into a pickable file, load it as local data parameter and check for equality.
In test_all_param_types, we dynamically created an EmptyClass, put it in aq.create_param(), and check for equality.
I didn't do what test_all_param_types did is because pickle.load complain a lot in pytest. Would that be enough of coverage?
There was a problem hiding this comment.
Can you share the error you were seeing after doing what test_all_param_types does.
There was a problem hiding this comment.
I'd rather we try to test this the same way of creating an empty class
There was a problem hiding this comment.
There was a problem hiding this comment.
I have created https://linear.app/aqueducthq/issue/ENG-2774/investigate-the-limitation-of-artifacttypepicklable-from-create-param. Feel free to give it a read to see if the issue makes sense
| Unable to check that the input is picklable, since `pickle.loads()` | ||
| complains about `import of module 'param_test' failed`. | ||
| """ | ||
| print(input) |
There was a problem hiding this comment.
Lets remove this print statement
| return input | ||
|
|
||
| with open("data/test.pickle", "wb") as file: | ||
| pickle.dump(ArtifactType, file) |
There was a problem hiding this comment.
Can you share the error you were seeing after doing what test_all_param_types does.
| return input | ||
|
|
||
| with open("data/test.pickle", "wb") as file: | ||
| pickle.dump(ArtifactType, file) |
There was a problem hiding this comment.
I'd rather we try to test this the same way of creating an empty class
| PICKLE = "pickle" | ||
| STRING = "string" | ||
| BYTES = "bytes" | ||
| TF_KERAS = "tensorflow-keras-model" |
There was a problem hiding this comment.
Can we also add a test case for TF_KERAS
sdk/aqueduct/utils/serialization.py
Outdated
| return Image.open(path) | ||
|
|
||
|
|
||
| def _read_local_json_content(path: str) -> Any: | ||
| with open(path, mode="rb", encoding=DEFAULT_ENCODING) as file: | ||
| return json.load(file.read()) |
There was a problem hiding this comment.
You should be able to do json.load(file) directly
…com:aqueducthq/aqueduct into eng-2585-allow-users-to-create-all-types-of
[## Describe your changes and why you are making these changes
client.create_param("name, default = "path/to/file", use_local=True, as_type= ArtifactType.xx). We extract the content of the file based on the type and serve it as content for parameter artifact.Testing:
The test
test_all_local_data_typesis similar totest_all_param_typesexcept that all the data is coming from local data.Related issue number (if any)
ENG-2585
Loom demo (if any)
Checklist before requesting a review
python3 scripts/run_linters.py -hfor usage).run_integration_test: Runs integration testsskip_integration_test: Skips integration tests (Should be used when changes are ONLY documentation/UI)](https://linear.app/aqueducthq/issue/ENG-2585/allow-users-to-create-all-types-of-parameters-from-local-data)