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

Fix ability to pass None to task with Optional kwarg, add test #1657

Merged
merged 1 commit into from
May 26, 2023

Conversation

fg91
Copy link
Member

@fg91 fg91 commented May 25, 2023

TL;DR

from flytekit import task, workflow, Workflow
from dataclasses import dataclass
from dataclasses_json import dataclass_json
from typing import Optional

@dataclass_json
@dataclass
class Config:
    lr: float = 0.1

@task
def func(cfg: Optional[Config] = None) -> None:
    print(cfg is None)

wf = Workflow(name="foo")
wf.add_entity(func, cfg=None)

wf()

This code snippet works up to flytekit version 1.5.0. It does not work in 1.6.0 (change introduced in this commit), failing with this error:

╭──────────────────────────────────────────────────── Traceback (most recent call last) ─────────────────────────────────────────────────────╮
│ /home/username/miniconda3/envs/debug/lib/python3.9/site-packages/flytekit/core/promise.py:1002 in create_and_link_node                   │
│                                                                                                                                            │
│ ❱ 1002 │   │   │   │   binding_from_python_std(                                                                                            │
│                                                                                                                                            │
│ /home/username/miniconda3/envs/debug/lib/python3.9/site-packages/flytekit/core/promise.py:690 in binding_from_python_std                 │
│                                                                                                                                            │
│ ❱  690 │   binding_data = binding_data_from_python_std(ctx, expected_literal_type, t_value, t_v                                            │
│                                                                                                                                            │
│ /home/username/miniconda3/envs/debug/lib/python3.9/site-packages/flytekit/core/promise.py:632 in binding_data_from_python_std            │
│                                                                                                                                            │
│ ❱  632 │   │   raise AssertionError(                                                                                                       │
╰────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
AssertionError: Failed to bind data None with literal type [<FlyteLiteral simple: STRUCT metadata { fields { key: "definitions" value { 
struct_value { fields { key: "ConfigSchema" value { struct_value { fields { key: "type" value { string_value: "object" } } fields { key: 
"properties" value { struct_value { fields { key: "lr" value { struct_value { fields { key: "type" value { string_value: "number" } } fields {
key: "title" value { string_value: "lr" } } fields { key: "format" value { string_value: "float" } } fields { key: "default" value { 
number_value: 0.1 } } } } } } } } fields { key: "additionalProperties" value { bool_value: false } } } } } } } } fields { key: "$schema" value
{ string_value: "http://json-schema.org/draft-07/schema#" } } fields { key: "$ref" value { string_value: "#/definitions/ConfigSchema" } } } 
structure { tag: "Object-Dataclass-Transformer" }>, <FlyteLiteral simple: NONE structure { tag: "none" }>].

The above exception was the direct cause of the following exception:

╭──────────────────────────────────────────────────── Traceback (most recent call last) ─────────────────────────────────────────────────────╮
│ /home/username/torch-experiments/none.py:18 in <module>                                                                                  │
│                                                                                                                                            │
│ ❱ 18 wf.add_entity(func, cfg=None)                                                                                                         │
│                                                                                                                                            │
│ /home/username/miniconda3/envs/debug/lib/python3.9/site-packages/flytekit/core/workflow.py:551 in add_entity                             │
│                                                                                                                                            │
│ ❱ 551 │   │   │   n = create_node(entity=entity, **kwargs)                                                                                 │
│                                                                                                                                            │
│ /home/username/miniconda3/envs/debug/lib/python3.9/site-packages/flytekit/core/node_creation.py:93 in create_node                        │
│                                                                                                                                            │
│ ❱  93 │   │   outputs = entity(**kwargs)                                                                                                   │
│                                                                                                                                            │
│ /home/username/miniconda3/envs/debug/lib/python3.9/site-packages/flytekit/core/base_task.py:304 in __call__                              │
│                                                                                                                                            │
│ ❱ 304 │   │   return flyte_entity_call_handler(self, *args, **kwargs)  # type: ignore                                                      │
│                                                                                                                                            │
│ /home/username/miniconda3/envs/debug/lib/python3.9/site-packages/flytekit/core/promise.py:1092 in flyte_entity_call_handler              │
│                                                                                                                                            │
│ ❱ 1092 │   │   return create_and_link_node(ctx, entity=entity, **kwargs)                                                                   │
│                                                                                                                                            │
│ /home/username/miniconda3/envs/debug/lib/python3.9/site-packages/flytekit/core/promise.py:1012 in create_and_link_node                   │
│                                                                                                                                            │
│ ❱ 1012 │   │   │   raise AssertionError(f"Failed to Bind variable {k} for function {entity.name                                            │
╰────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
AssertionError: Failed to Bind variable cfg for function func.

I assume the elif case should not be entered if the value is None. Is this correct @pingsutw ?

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/

Signed-off-by: Fabio Grätz <fabiogratz@googlemail.com>
@fg91 fg91 force-pushed the fabio/fix/optional-kwargs branch from 1c727cd to 90160ee Compare May 25, 2023 15:33
@fg91 fg91 self-assigned this May 25, 2023
@fg91 fg91 added the bug Something isn't working label May 25, 2023
@codecov
Copy link

codecov bot commented May 25, 2023

Codecov Report

Merging #1657 (90160ee) into master (eafcc82) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1657      +/-   ##
==========================================
+ Coverage   71.01%   71.03%   +0.01%     
==========================================
  Files         336      336              
  Lines       30756    30765       +9     
  Branches     5572     5573       +1     
==========================================
+ Hits        21842    21853      +11     
+ Misses       8368     8367       -1     
+ Partials      546      545       -1     
Impacted Files Coverage Δ
flytekit/core/promise.py 54.72% <100.00%> (ø)
tests/flytekit/unit/core/test_promise.py 90.81% <100.00%> (+0.92%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

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

Good catch, thank you @fg91

@pingsutw pingsutw merged commit f671fb6 into master May 26, 2023
123 of 124 checks passed
eapolinario pushed a commit that referenced this pull request Jul 10, 2023
Signed-off-by: Fabio Grätz <fabiogratz@googlemail.com>
Co-authored-by: Fabio Grätz <fabiogratz@googlemail.com>
eapolinario added a commit that referenced this pull request Jul 12, 2023
* Multi arch imageSpec (#1630)

Multi arch imageSpec (#1630)

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

* Add executor_path and applications_path to spark config (#1634)

* Add executor_path and applications_path to spark config

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

---------

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Add support for env vars to pyflyte run (#1617)

* Add support for env vars to pyflyte run

Signed-off-by: Kevin Su <pingsutw@apache.org>

* bump idl

Signed-off-by: Kevin Su <pingsutw@apache.org>

* update doc

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

---------

Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

* Fetch task executions in dynamic  (#1636)

* fetch task executions in dynamic

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

* lint

Signed-off-by: Kevin Su <pingsutw@apache.org>

---------

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Added metrics command to pyflyte (#1513)

Signed-off-by: Daniel Rammer <daniel@union.ai>

* Add http_proxy to client & Fix deviceflow (#1611)

* Add http_proxy to client & Fix deviceflow

RB=3890720

Signed-off-by: byhsu <byhsu@linkedin.com>

* nit

Signed-off-by: byhsu <byhsu@linkedin.com>

* lint!

Signed-off-by: byhsu <byhsu@linkedin.com>

---------

Signed-off-by: byhsu <byhsu@linkedin.com>
Co-authored-by: byhsu <byhsu@linkedin.com>
Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

* Improve variable names (#1642)

Signed-off-by: byhsu <byhsu@linkedin.com>
Co-authored-by: byhsu <byhsu@linkedin.com>

* Address resolution (#1567)

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>

* pyflyte run supports pickle (#1646)

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Wait for the pod plugin instead of flytekit (#1647)

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>
Co-authored-by: eduardo apolinario <eapolinario@users.noreply.github.com>

* Beautify deviceflow prompt (#1625)

* Beautify deviceflow prompt

Signed-off-by: byhsu <byhsu@linkedin.com>

* lint!

Signed-off-by: byhsu <byhsu@linkedin.com>

* lint

Signed-off-by: byhsu <byhsu@linkedin.com>

---------

Signed-off-by: byhsu <byhsu@linkedin.com>
Co-authored-by: byhsu <byhsu@linkedin.com>

* Improve flytekit register (#1643)

* Fix pyflyte register

Signed-off-by: byhsu <byhsu@linkedin.com>

* revert

Signed-off-by: byhsu <byhsu@linkedin.com>

* lint

Signed-off-by: byhsu <byhsu@linkedin.com>

---------

Signed-off-by: byhsu <byhsu@linkedin.com>
Co-authored-by: byhsu <byhsu@linkedin.com>

* Pass verify flag to all authenticators (#1641)

Signed-off-by: byhsu <byhsu@linkedin.com>

* Allow annotated FlyteFile as task input argument (#1632)

* fix: Allow annotated FlyteFile as task input argument

Using an annotated FlyteFile type as an input to a task was previously impossible due
to an exception being raised in `FlyteFilePathTransformer.to_python_value`.

This commit applies the fix previously used in `FlyteFilePathTransformer.to_literal`
to permit using annotated FlyteFiles as either inputs and outputs of a task.

Issue: #3424
Signed-off-by: Adrian Rumpold <a.rumpold@gmail.com>

* refactor: Unified handling of annotated types in type engine

Issue: #3424
Signed-off-by: Adrian Rumpold <a.rumpold@gmail.com>

* fix: Use py3.8-compatible types in type engine tests

Issue: #3424
Signed-off-by: Adrian Rumpold <a.rumpold@gmail.com>

---------

Signed-off-by: Adrian Rumpold <a.rumpold@gmail.com>

* Use logger instead of print statement in sqlalchemy plugin (#1651)

* use logging info instead of print

Signed-off-by: wirthual <wirthra@gmail.com>

* isorted files

Signed-off-by: wirthual <wirthra@gmail.com>

* import root logger from flytekit

Signed-off-by: wirthual <wirthra@gmail.com>

---------

Signed-off-by: wirthual <wirthra@gmail.com>

* Map over notebook task (#1650)

* map over notebook

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

* tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

* add a flag

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

* fix tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* nit

Signed-off-by: Kevin Su <pingsutw@apache.org>

* lint

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Fix tests

Signed-off-by: Kevin Su <pingsutw@apache.org>

* lint

Signed-off-by: Kevin Su <pingsutw@apache.org>

---------

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Support single literals in tiny url (#1654)

Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>

* Add support overriding image (#1652)

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Fix ability to pass None to task with Optional kwarg, add test (#1657)

Signed-off-by: Fabio Grätz <fabiogratz@googlemail.com>
Co-authored-by: Fabio Grätz <fabiogratz@googlemail.com>

* Regenerate plugins requirements

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>

* Regenerate plugins requirements and linting

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>

* Regenerate whylogs requirements

Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>

---------

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Daniel Rammer <daniel@union.ai>
Signed-off-by: byhsu <byhsu@linkedin.com>
Signed-off-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Signed-off-by: eduardo apolinario <eapolinario@users.noreply.github.com>
Signed-off-by: Adrian Rumpold <a.rumpold@gmail.com>
Signed-off-by: wirthual <wirthra@gmail.com>
Signed-off-by: Fabio Grätz <fabiogratz@googlemail.com>
Co-authored-by: Kevin Su <pingsutw@apache.org>
Co-authored-by: Dan Rammer <daniel@union.ai>
Co-authored-by: ByronHsu <byronhsu1230@gmail.com>
Co-authored-by: byhsu <byhsu@linkedin.com>
Co-authored-by: Yee Hing Tong <wild-endeavor@users.noreply.github.com>
Co-authored-by: eduardo apolinario <eapolinario@users.noreply.github.com>
Co-authored-by: Adrian Rumpold <a.rumpold@gmail.com>
Co-authored-by: wirthual <wirthra@gmail.com>
Co-authored-by: Fabio M. Graetz, Ph.D <fabiograetz@googlemail.com>
Co-authored-by: Fabio Grätz <fabiogratz@googlemail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants