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

Enable using positional args for workflow with default value #3149

Merged
merged 5 commits into from
Feb 22, 2025

Conversation

machichima
Copy link
Contributor

@machichima machichima commented Feb 21, 2025

Tracking issue

flyteorg/flyte#6208

Why are the changes needed?

Remove the value in input_kwargs (which contains the default value) if the value is set in args, to prevent passing kwargs and args both for same argument, which cause the error described in the issue.

What changes were proposed in this pull request?

How was this patch tested?

Add test test_positional_args_workflow_with_default_value extend from test_positional_args_task for positional arguments with default value set

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

Summary by Bito

This PR addresses subworkflow argument handling by preventing simultaneous use of positional arguments and default values. The workflow execution logic has been modified to properly handle input_kwargs when positional arguments are provided. Test coverage has been enhanced to validate subworkflow behavior, including test cases for mixed positional and default arguments scenarios, extra arguments handling, and keyword arguments validation.

Unit tests added: True

Estimated effort to review (1-5, lower is better): 2

so that in flyte_entity_call_handler the kwargs will not contain args

Signed-off-by: machichima <[email protected]>
@flyte-bot
Copy link
Contributor

flyte-bot commented Feb 21, 2025

Code Review Agent Run #a46a9e

Actionable Suggestions - 1
  • flytekit/core/workflow.py - 1
Review Details
  • Files reviewed - 2 · Commit Range: b823ba0..50af9c3
    • flytekit/core/workflow.py
    • tests/flytekit/unit/core/test_serialization.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by Bito Logo

@machichima machichima changed the title 6208 subworkflow args Enable using positional args for workflow with default value Feb 21, 2025
@flyte-bot
Copy link
Contributor

flyte-bot commented Feb 21, 2025

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
Feature Improvement - Enhanced Workflow Argument Handling

workflow.py - Added validation for positional arguments and proper handling of default values in workflow calls

test_serialization.py - Added comprehensive tests for workflow positional arguments with default values and argument validation

Copy link
Collaborator

@eapolinario eapolinario 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, this looks pretty good, just a couple of nits.

Comment on lines 1005 to 1007
@workflow
def wf_mixed_positional_and_keyword_args() -> int:
return sub_wf(arg1, y=arg2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you also add a case where we invoke sub_wf with a mix of positional args and default values? For example:

    @workflow
    def wf_mixed_positional_and_default_args() -> int:
        return sub_wf(arg1)

Copy link
Member

@Future-Outlier Future-Outlier left a comment

Choose a reason for hiding this comment

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

lgtm, just use debugger to check the whole process, thank you @machichima

@Future-Outlier Future-Outlier enabled auto-merge (squash) February 22, 2025 06:20
@Future-Outlier Future-Outlier merged commit 93c87c3 into flyteorg:master Feb 22, 2025
112 checks passed
@flyte-bot
Copy link
Contributor

flyte-bot commented Feb 22, 2025

Code Review Agent Run #abe196

Actionable Suggestions - 2
  • tests/flytekit/unit/core/test_serialization.py - 2
Review Details
  • Files reviewed - 1 · Commit Range: 50af9c3..ced38bd
    • tests/flytekit/unit/core/test_serialization.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by Bito Logo

@eapolinario
Copy link
Collaborator

This looks great, thank you!

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.

4 participants