feat(bindings): Task arguments default value binding #2401

MortalHappiness · 2024-05-09T17:53:52Z

Tracking issue

Why are the changes needed?

See the issue description for details.

What changes were proposed in this pull request?

if the key is not in kwargs but in interface.inputs_with_defaults, add the value in interface.inputs_with_defaults to kwargs.

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

I updated the documentation accordingly.
All new and existing tests passed.
All commits are signed-off.

Related PRs

Docs link

pingsutw

Also, should we remove these lines? since if statement will always be false.

tests/flytekit/unit/core/test_promise.py

MortalHappiness · 2024-05-15T12:49:20Z

Also, should we remove these lines? since if statement will always be false.

Done

wild-endeavor · 2024-05-17T01:05:32Z

could you add more tests please? take a look at serialization tests, it should be pretty simple to test the compiled workflow.

do we do anything special for [] and {}? You're not supposed to use those as function defaults anyways but some people might.

MortalHappiness · 2024-05-17T13:25:58Z

could you add more tests please? take a look at serialization tests, it should be pretty simple to test the compiled workflow.

do we do anything special for [] and {}? You're not supposed to use those as function defaults anyways but some people might.

Done

codecov · 2024-05-26T15:00:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.56%. Comparing base (57ee143) to head (2de588f).
Report is 1 commits behind head on master.

❗ Current head 2de588f differs from pull request most recent head b92519a

Please upload reports for the commit b92519a to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2401      +/-   ##
==========================================
+ Coverage   72.28%   78.56%   +6.27%     
==========================================
  Files         182      185       +3     
  Lines       18491    18603     +112     
  Branches     3832     3604     -228     
==========================================
+ Hits        13367    14616    +1249     
+ Misses       4488     3348    -1140     
- Partials      636      639       +3

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

wild-endeavor

thanks! just minor comments. can you take a look at the failing unit test also? for some performance reason, we split out tests that rely on pandas into a separate gh action. Take a look at

flytekit/tests/flytekit/unit/core/test_artifacts.py

Line 26 in 91cfb97

if "pandas" not in sys.modules:

wild-endeavor · 2024-05-27T23:40:00Z

tests/flytekit/unit/core/test_serialization.py

@@ -84,6 +122,287 @@ def raw_container_wf(val1: int, val2: int) -> int:
    assert sum_spec.template.container.image == "alpine"


+@pytest.mark.parametrize(


I know some people promote these, but once you get past a few args, I find these parametrized tests super hard to follow. Could you maybe split up the test below into multiple tests? That would reduce the need to have so many args - both help make it more readable, and separate testing concerns better.

flytekit/core/promise.py

tests/flytekit/unit/core/test_dynamic.py

flytekit/core/promise.py

wild-endeavor

still need to look at monodocs. i think we can do that later today.

wild-endeavor · 2024-05-29T18:09:01Z

eduardo fixed the build separately https://github.com/flyteorg/flyte/pull/5425/files can merge this after passing

wild-endeavor · 2024-05-29T18:12:31Z

tests/flytekit/unit/core/test_serialization.py

+    wf_no_input_spec = get_serializable(OrderedDict(), serialization_settings, wf_no_input)
+    wf_with_input_spec = get_serializable(OrderedDict(), serialization_settings, wf_with_input)
+
+    assert wf_no_input_spec.template.nodes[0].inputs[0].binding.value == Scalar(


actually sorry, i think we need to think about this case a bit more.

Do you mean I need to bind this to Union instead of Primitive?

Done. See the latest commit for details. Here I added a bool argument is_union_type_variants_expanded. And if the default argument is used, then is_union_type_variants_expanded will be set to False, which will bypass the for loop expansion for union types here

flytekit/flytekit/core/promise.py

Lines 706 to 718 in c9cb9df

elif t_value is not None and is_union_type_variants_expanded and expected_literal_type.union_type is not None:

for i in range(len(expected_literal_type.union_type.variants)):

try:

lt_type = expected_literal_type.union_type.variants[i]

python_type = get_args(t_value_type)[i] if t_value_type else None

return binding_data_from_python_std(ctx, lt_type, t_value, python_type, nodes)

except Exception:

logger.debug(

f"failed to bind data {t_value} with literal type {expected_literal_type.union_type.variants[i]}."

)

raise AssertionError(

f"Failed to bind data {t_value} with literal type {expected_literal_type.union_type.variants}."

)

Also, I added two tests for testing Optional[List[int]].

wild-endeavor · 2024-05-29T18:12:53Z

sorry, missed something, let me get back to you later.

wild-endeavor · 2024-06-05T20:46:25Z

flytekit/core/promise.py

@@ -684,6 +690,7 @@ def binding_data_from_python_std(
    t_value: Any,
    t_value_type: type,
    nodes: List[Node],
+    is_union_type_variants_expanded: bool = True,


can we remove this now with the other change?

Resolves: flyteorg/flyte#5321 Signed-off-by: Chi-Sheng Liu <[email protected]>

eapolinario · 2024-06-06T17:13:12Z

This is pretty awesome! Thank you for all the hard work here, @MortalHappiness and @wild-endeavor !

flyteorg/flyte#5321 if the key is not in `kwargs` but in `interface.inputs_with_defaults`, add the value in `interface.inputs_with_defaults` to `kwargs`. Signed-off-by: Chi-Sheng Liu <[email protected]> Signed-off-by: bugra.gedik <[email protected]>

flyteorg/flyte#5321 if the key is not in `kwargs` but in `interface.inputs_with_defaults`, add the value in `interface.inputs_with_defaults` to `kwargs`. Signed-off-by: Chi-Sheng Liu <[email protected]> Signed-off-by: Jan Fiedler <[email protected]>

eapolinario · 2024-08-02T21:59:33Z

flytekit/core/promise.py

+                if not isinstance(default_val, Hashable):
+                    raise _user_exceptions.FlyteAssertion("Cannot use non-hashable object as default argument")


Why do we need this extra-check? Any value that can't be handled by the existing type transformers at registration time is going to show pretty prominently, right?

This is used to prevent the confusing behavior of mutable default arguments.

https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments

Relying on Hashable prevents dataclasses from being used as default arguments. This is too restrictive, since even sophisticated linters like ruff are not able to catch this example:

from dataclasses import dataclass from mashumaro import DataClassDictMixin @dataclass class DC(DataClassDictMixin): a: int b: str def f(dc: DC = DC(42, "43"), xs: list[int] = [1,2], d: dict[str, int] = {"abc": 42}, s: set(int) = set([1,2,3])): ... if __name__ == "__main__": f()

Running ruff with https://docs.astral.sh/ruff/rules/mutable-argument-default/ turned on returns:

❯ ruff check example example/dc.py:9:46: B006 Do not use mutable data structures for argument defaults | 7 | b: str 8 | 9 | def f(dc: DC = DC(42, "43"), xs: list[int] = [1,2], d: dict[str, int] = {"abc": 42}, s: set(int) = set([1,2,3])): | ^^^^^ B006 10 | ... | = help: Replace with `None`; initialize within function example/dc.py:9:73: B006 Do not use mutable data structures for argument defaults | 7 | b: str 8 | 9 | def f(dc: DC = DC(42, "43"), xs: list[int] = [1,2], d: dict[str, int] = {"abc": 42}, s: set(int) = set([1,2,3])): | ^^^^^^^^^^^ B006 10 | ... | = help: Replace with `None`; initialize within function example/dc.py:9:100: B006 Do not use mutable data structures for argument defaults | 7 | b: str 8 | 9 | def f(dc: DC = DC(42, "43"), xs: list[int] = [1,2], d: dict[str, int] = {"abc": 42}, s: set(int) = set([1,2,3])): | ^^^^^^^^^^^^ B006 10 | ... | = help: Replace with `None`; initialize within function Found 3 errors. No fixes available (3 hidden fixes can be enabled with the `--unsafe-fixes` option).

I propose we handle these 3 common errors explicitly instead. This is also going to simplify the error message. I'll get a PR out soon.

@eapolinario I saw your PR. Quite make senses for me. Those 3 container types are indeed the most commonly used non-hashable types.

How about collections.deque and collections.ChainMap?

MortalHappiness requested review from wild-endeavor, kumare3, eapolinario, pingsutw, cosmicBboy and samhita-alla as code owners May 9, 2024 17:53

MortalHappiness marked this pull request as draft May 9, 2024 18:04

MortalHappiness force-pushed the feature/#5321-task-default-value-binding branch from b434ddf to cc13415 Compare May 9, 2024 18:45

MortalHappiness marked this pull request as ready for review May 9, 2024 18:46

MortalHappiness marked this pull request as draft May 9, 2024 18:50

MortalHappiness marked this pull request as ready for review May 10, 2024 04:51

pingsutw reviewed May 14, 2024

View reviewed changes

tests/flytekit/unit/core/test_promise.py Outdated Show resolved Hide resolved

MortalHappiness force-pushed the feature/#5321-task-default-value-binding branch from cc13415 to 277d88b Compare May 15, 2024 12:49

MortalHappiness force-pushed the feature/#5321-task-default-value-binding branch 2 times, most recently from 51e15e5 to 6446ad1 Compare May 16, 2024 15:13

MortalHappiness force-pushed the feature/#5321-task-default-value-binding branch from 6446ad1 to 6c808ae Compare May 17, 2024 13:24

MortalHappiness force-pushed the feature/#5321-task-default-value-binding branch 7 times, most recently from 545adbc to d9a7345 Compare May 26, 2024 09:33

wild-endeavor reviewed May 28, 2024

View reviewed changes

MortalHappiness marked this pull request as draft May 29, 2024 05:37

MortalHappiness force-pushed the feature/#5321-task-default-value-binding branch from d9a7345 to c90ce87 Compare May 29, 2024 11:37

MortalHappiness marked this pull request as ready for review May 29, 2024 11:38

MortalHappiness force-pushed the feature/#5321-task-default-value-binding branch from c90ce87 to 2de588f Compare May 29, 2024 12:33

wild-endeavor previously approved these changes May 29, 2024

View reviewed changes

wild-endeavor reviewed May 29, 2024

View reviewed changes

MortalHappiness dismissed wild-endeavor’s stale review via c9cb9df June 4, 2024 15:32

MortalHappiness force-pushed the feature/#5321-task-default-value-binding branch from 2de588f to c9cb9df Compare June 4, 2024 15:32

wild-endeavor mentioned this pull request Jun 4, 2024

Add escape for scalars to binding during union handling #2460

Merged

3 tasks

MortalHappiness mentioned this pull request Jun 5, 2024

[Docs] Prevent using mutable default arguments in flytesnacks flyteorg/flyte#5383

Closed

2 tasks

wild-endeavor reviewed Jun 5, 2024

View reviewed changes

MortalHappiness added 5 commits June 6, 2024 12:10

feat(bindings): Task arguments default value binding

ac1b026

Resolves: flyteorg/flyte#5321 Signed-off-by: Chi-Sheng Liu <[email protected]>

chore: Delete redundant lines

7279f33

Resolves: flyteorg/flyte#5321 Signed-off-by: Chi-Sheng Liu <[email protected]>

test(default-args): Add more tests according to Yee's recommendation

51a97d3

Resolves: flyteorg/flyte#5321 Signed-off-by: Chi-Sheng Liu <[email protected]>

feat(bindings): Don't expand union type if default arg is used

b45a2aa

Resolves: flyteorg/flyte#5321 Signed-off-by: Chi-Sheng Liu <[email protected]>

fix(bindings): Update according to PR flyteorg#2460

b92519a

Resolves: flyteorg/flyte#5321 Signed-off-by: Chi-Sheng Liu <[email protected]>

MortalHappiness force-pushed the feature/#5321-task-default-value-binding branch from c9cb9df to b92519a Compare June 6, 2024 04:17

wild-endeavor approved these changes Jun 6, 2024

View reviewed changes

wild-endeavor merged commit 47f2a29 into flyteorg:master Jun 6, 2024
46 checks passed

eapolinario reviewed Aug 2, 2024

View reviewed changes

eapolinario mentioned this pull request Aug 5, 2024

Handle common cases of mutable default arguments explicitly #2651

Merged

3 tasks

Future-Outlier mentioned this pull request Aug 10, 2024

Nominate Chi-Sheng Liu (MortalHappiness) as Flyte committer flyteorg/community#30

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(bindings): Task arguments default value binding #2401

feat(bindings): Task arguments default value binding #2401

MortalHappiness commented May 9, 2024 •

edited

Loading

pingsutw left a comment

MortalHappiness commented May 15, 2024

wild-endeavor commented May 17, 2024

MortalHappiness commented May 17, 2024

codecov bot commented May 26, 2024 •

edited

Loading

wild-endeavor left a comment

wild-endeavor May 27, 2024

MortalHappiness May 29, 2024

wild-endeavor left a comment

wild-endeavor commented May 29, 2024

wild-endeavor May 29, 2024

MortalHappiness May 30, 2024

wild-endeavor Jun 3, 2024

MortalHappiness Jun 4, 2024 •

edited

Loading

wild-endeavor commented May 29, 2024

wild-endeavor Jun 5, 2024

MortalHappiness Jun 6, 2024

eapolinario commented Jun 6, 2024

eapolinario Aug 2, 2024

MortalHappiness Aug 4, 2024

eapolinario Aug 5, 2024

eapolinario Aug 5, 2024

MortalHappiness Aug 6, 2024

MortalHappiness Aug 6, 2024 •

edited

Loading

		@@ -84,6 +122,287 @@ def raw_container_wf(val1: int, val2: int) -> int:
		assert sum_spec.template.container.image == "alpine"


		@pytest.mark.parametrize(

	elif t_value is not None and is_union_type_variants_expanded and expected_literal_type.union_type is not None:
	for i in range(len(expected_literal_type.union_type.variants)):
	try:
	lt_type = expected_literal_type.union_type.variants[i]
	python_type = get_args(t_value_type)[i] if t_value_type else None
	return binding_data_from_python_std(ctx, lt_type, t_value, python_type, nodes)
	except Exception:
	logger.debug(
	f"failed to bind data {t_value} with literal type {expected_literal_type.union_type.variants[i]}."
	)
	raise AssertionError(
	f"Failed to bind data {t_value} with literal type {expected_literal_type.union_type.variants}."
	)

		if not isinstance(default_val, Hashable):
		raise _user_exceptions.FlyteAssertion("Cannot use non-hashable object as default argument")

feat(bindings): Task arguments default value binding #2401

feat(bindings): Task arguments default value binding #2401

Conversation

MortalHappiness commented May 9, 2024 • edited Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

Related PRs

Docs link

pingsutw left a comment

Choose a reason for hiding this comment

MortalHappiness commented May 15, 2024

wild-endeavor commented May 17, 2024

MortalHappiness commented May 17, 2024

codecov bot commented May 26, 2024 • edited Loading

Codecov Report

wild-endeavor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wild-endeavor left a comment

Choose a reason for hiding this comment

wild-endeavor commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MortalHappiness Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

wild-endeavor commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eapolinario commented Jun 6, 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

Choose a reason for hiding this comment

MortalHappiness Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

MortalHappiness commented May 9, 2024 •

edited

Loading

codecov bot commented May 26, 2024 •

edited

Loading

MortalHappiness Jun 4, 2024 •

edited

Loading

MortalHappiness Aug 6, 2024 •

edited

Loading