-
Notifications
You must be signed in to change notification settings - Fork 314
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
Gatenode local execution, was parsing the input twice #3101
Conversation
Signed-off-by: Ketan Umare <[email protected]>
Code Review Agent Run #a13ba8Actionable Suggestions - 1
Review Details
|
Changelist by BitoThis pull request implements the following key changes.
|
option = click.Option(["--input"], type=literal_converter.click_type) | ||
v = literal_converter.click_type.convert(user_input, option, click.Context(command=click.Command(""))) | ||
return TypeEngine.to_literal(FlyteContext.current_context(), v, t, lt) | ||
return TypeEngine.to_literal(FlyteContext.current_context(), user_input, t, lt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider if the direct conversion of user_input
to literal is safe without the intermediate click type conversion. The removed click type validation could have been providing important input validation.
Code suggestion
Check the AI-generated fix before applying
return TypeEngine.to_literal(FlyteContext.current_context(), user_input, t, lt) | |
option = click.Option(["--input"], type=literal_converter.click_type) | |
validated_input = literal_converter.click_type.convert(user_input, option, click.Context(command=click.Command(""))) | |
return TypeEngine.to_literal(FlyteContext.current_context(), validated_input, t, lt) |
Code Review Run #a13ba8
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3101 +/- ##
===========================================
+ Coverage 51.81% 77.91% +26.09%
===========================================
Files 202 202
Lines 21469 21470 +1
Branches 2766 2767 +1
===========================================
+ Hits 11125 16728 +5603
+ Misses 9735 3943 -5792
- Partials 609 799 +190 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import flytekit as fl
from dataclasses import dataclass
from datetime import timedelta
from pydantic import BaseModel
dt = timedelta(seconds=60)
class MyClass(BaseModel):
x: int
y: str
@fl.task
def my_task(c: MyClass) -> MyClass:
return c
@fl.workflow
def my_wf():
c = fl.wait_for_input("tst", dt, MyClass)
my_task(c=c)
(flyte-dev) future@outlier ~ % pyflyte run build/PR/pr_ketan_gatenode.py my_wf
10:42:38.994067 INFO file.py:252 - Using flytectl/YAML config /Users/future-outlier/.flyte/config-sandbox.yaml
Running Execution on local.
[Input Gate] Waiting for input @tst of type <class 'pr_ketan_gatenode.MyClass'>: {"x":1, "y":"2"}
10:42:43.650104 INFO utils.py:344 - AsyncTranslate literal to python value. [Time: 0.000003s]
10:42:43.650933 INFO utils.py:344 - Translate literal to python value. [Time: 0.000845s]
10:42:43.651410 INFO base_task.py:749 - Invoking pr_ketan_gatenode.my_task with inputs: {'c': MyClass(x=1, y='2')}
10:42:43.651826 INFO utils.py:344 - Execute user level code. [Time: 0.000006s]
10:42:43.652375 INFO utils.py:344 - Translate the output to literals. [Time: 0.000146s]
10:42:43.652871 INFO utils.py:344 - dispatch execute. [Time: 0.000726s]
Signed-off-by: Ketan Umare <[email protected]> Co-authored-by: Ketan Umare <[email protected]> Signed-off-by: Umer Ahmad <[email protected]>
For example like so,
Local execution will result in failure
This fixes the local execution. Remote works fine!
Summary by Bito
Fixed a bug in local execution where input parsing was being performed twice for gatenode interactions. The change simplifies input parsing by removing redundant type conversion in parse_stdin.py, improving handling of complex types like custom dataclasses during local workflow execution while maintaining remote execution behavior.Unit tests added: False
Estimated effort to review (1-5, lower is better): 1