[Feature] Support positional arguments #2522
Conversation
MortalHappiness
requested review from
wild-endeavor,
kumare3,
eapolinario,
pingsutw,
cosmicBboy and
samhita-alla
as code owners
MortalHappiness
force-pushed
the
feature/#5320-positional-args
branch
from
a247817
to
ccccaea
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2522 +/- ##
===========================================
+ Coverage 71.79% 91.19% +19.39%
===========================================
Files 182 75 -107
Lines 18561 3849 -14712
Branches 3654 0 -3654
===========================================
- Hits 13326 3510 -9816
+ Misses 4592 339 -4253
+ Partials 643 0 -643 ☔ View full report in Codecov by Sentry. |
MortalHappiness
force-pushed
the
feature/#5320-positional-args
branch
2 times, most recently
from
be4c921
to
0e46066
Compare
flytekit/core/base_sql_task.py
Outdated
| @@ -24,9 +25,9 @@ def __init__( | |||
| query_template: str, | |||
| task_config: Optional[T] = None, | |||
| task_type="sql_task", | |||
| inputs: Optional[Dict[str, Tuple[Type, Any]]] = None, | |||
There was a problem hiding this comment.
Changed in version 3.7: Dictionary order is guaranteed to be insertion order. This behavior was an implementation detail of CPython from 3.6.
https://docs.python.org/3.7/library/stdtypes.html#dict
flytekit only supports 3.8+, do we still need to use OrderedDict here?
There was a problem hiding this comment.
There was a problem hiding this comment.
I know that the dictionary after Python 3.7 is ordered. I use OrderedDict because I found somewhere in the codebase still uses OrderedDict. After some discussions in meetings, it seems that those are legacy codes. Therefore, I've rolled back the changes of OrderedDict.
|
Could you also test it on serverless? Thanks! |
|
@MortalHappiness for changes that only touch flytekit, and to give all reviewers peace of mind, could you please sign up for serverless, use this commit to build the image, and run some tests? (incidentally, this also helps us test serverless 😄). Could you link it in the pr description as well please? |
|
|
||
| @workflow | ||
| def wf_pure_positional_args() -> int: | ||
| return t1(arg1, arg2) |
There was a problem hiding this comment.
could you also write a test (and run on serverless) where a downstream task takes both positional and named arguments from both 1) workflow inputs and 2) outputs of upstream tasks?
There was a problem hiding this comment.
Done. Please see if these tests are what you want. Thanks. Also, I'll run them on the serverless later.
Resolves: flyteorg/flyte#5320 Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
Resolves: flyteorg/flyte#5320 Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com>
MortalHappiness
force-pushed
the
feature/#5320-positional-args
branch
from
0e46066
to
3fd412a
Compare
|
from flytekit import task, workflow
@task
def t1(x: int, y: int) -> int:
return x + y * 2
@workflow
def wf() -> int:
return t1(5, y=6) |
- Change the `inputs` and `outputs` attributes in the `Interface` class to `OrderedDict` to preserve the order. - Write values in positional arguments to `kwargs`. Resolves: flyteorg/flyte#5320 Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com> Signed-off-by: bugra.gedik <bugra.gedik@predera.ai>
Tracking issue
Resolves: flyteorg/flyte#5320
Why are the changes needed?
The the issue description for details.
What changes were proposed in this pull request?
inputsandoutputsattributes in theInterfaceclass toOrderedDictto preserve the order.kwargs.How was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link