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

Do not deserialize default list/dict inputs using JSON in pyflyte run #1033

Merged
merged 1 commit into from
Jun 1, 2022

Conversation

samhita-alla
Copy link
Contributor

@samhita-alla samhita-alla commented Jun 1, 2022

Signed-off-by: Samhita Alla [email protected]

TL;DR

Default list/dict arguments are being deserialized by JSON when using the pyflyte run command resulting in a JSON type error. The arguments should be deserialized only when sent as command-line arguments.

Related Slack conversation:

Is it not possible to have default arguments for lists on workflows? eg. when i try to pyflyte run on
@workflow
def wf(
total_samples: List[int] = [16, 32, 64, 256],
):
I get TypeError: the JSON object must be str, bytes or bytearray, not list but without specifying a default it does work (and i can pass it to pyflyte as a string json list)

https://flyte-org.slack.com/archives/CNMKCU6FR/p1654084316153379

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

https://github.com/flyteorg/flyte/issues/

Follow-up issue

NA
OR
https://github.com/flyteorg/flyte/issues/

…AND [ARGS]...

  This command can execute either a workflow or a task from the commandline,
  for fully self-contained scripts. Tasks and workflows cannot be imported
  from other files currently. Please use `pyflyte package` or `pyflyte
  register` to handle those and then launch from the Flyte UI or `flytectl`

  Note: This command only works on regular Python packages, not namespace
  packages. When determining       the root of your project, it finds the
  first folder that does not have an __init__.py file.

Options:
  --remote                Whether to register and run the workflow on a Flyte
                          deployment
  -p, --project TEXT      Project to register and run this workflow in
  -d, --domain TEXT       Domain to register and run this workflow in
  --name TEXT             Name to assign to this execution
  --destination-dir TEXT  Directory inside the image where the tar file
                          containing the code will be copied to
  -i, --image TEXT        Image used to register and run.
  --service-account TEXT  Service account used when executing this workflow
  --wait-execution        Whether wait for the execution to finish
  --dump-snippet          Whether dump a code snippet instructing how to load
                          the workflow execution using flyteremote
  --help                  Show this message and exit.

Commands:
  setup.py  Run a [workflow|task] in a file using script mode
Signed-off-by: Samhita Alla <[email protected]>
@codecov
Copy link

codecov bot commented Jun 1, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.51%. Comparing base (771aa8a) to head (ccd2cda).
Report is 1068 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1033   +/-   ##
=======================================
  Coverage   86.50%   86.51%           
=======================================
  Files         265      267    +2     
  Lines       24773    24790   +17     
  Branches     2787     2787           
=======================================
+ Hits        21429    21446   +17     
  Misses       2867     2867           
  Partials      477      477           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@samhita-alla samhita-alla changed the title Capture default list/dict inputs in pyflyte run Do not deserialize default list/dict inputs using JSON in pyflyte run Jun 1, 2022
Copy link
Contributor

@kumare3 kumare3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

@eapolinario eapolinario merged commit c4056db into master Jun 1, 2022
eapolinario pushed a commit that referenced this pull request Jun 17, 2022
…AND [ARGS]... (#1033)

This command can execute either a workflow or a task from the commandline,
  for fully self-contained scripts. Tasks and workflows cannot be imported
  from other files currently. Please use `pyflyte package` or `pyflyte
  register` to handle those and then launch from the Flyte UI or `flytectl`

  Note: This command only works on regular Python packages, not namespace
  packages. When determining       the root of your project, it finds the
  first folder that does not have an __init__.py file.

Options:
  --remote                Whether to register and run the workflow on a Flyte
                          deployment
  -p, --project TEXT      Project to register and run this workflow in
  -d, --domain TEXT       Domain to register and run this workflow in
  --name TEXT             Name to assign to this execution
  --destination-dir TEXT  Directory inside the image where the tar file
                          containing the code will be copied to
  -i, --image TEXT        Image used to register and run.
  --service-account TEXT  Service account used when executing this workflow
  --wait-execution        Whether wait for the execution to finish
  --dump-snippet          Whether dump a code snippet instructing how to load
                          the workflow execution using flyteremote
  --help                  Show this message and exit.

Commands:
  setup.py  Run a [workflow|task] in a file using script mode
Signed-off-by: Samhita Alla <[email protected]>
@eapolinario eapolinario mentioned this pull request Jun 17, 2022
8 tasks
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.

3 participants