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

feat: support positional args for ref entity #3104

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

machichima
Copy link
Contributor

@machichima machichima commented Feb 2, 2025

Tracking issue

Similar to issue: flyteorg/flyte#5320, but for reference entities

Why are the changes needed?

Support positional args for reference task, workflow, and launch plan.

Originally, following code will fail with error like: FlyteAssertion: USER:AssertionError: error=Cannot call reference entity with args - detected 2 positional args (Promise(node:.x.[]), Promise(node:.y.[]))

@reference_task(
    project="flytesnacks",
    domain="development",
    name="ref_workflow.base_list_adder",
    version="FmnTvGOpEnjm2XrxeYF7EA",
)
def base_list_adder(x: list[int], y: list[int]) -> list[int]:
    ...

@workflow
def wf(x: list[int], y: list[int]) -> list[int]:
    return base_list_adder(x, y)

What changes were proposed in this pull request?

Write values in positional arguments (args) into 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

Summary by Bito

This PR enhances reference entities (tasks, workflows, and launch plans) by adding support for positional arguments. The implementation includes argument validation, conversion of positional to keyword arguments, and removes restrictions on using positional arguments. The changes maintain backward compatibility while providing more flexible argument passing options.

Unit tests added: False

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

@flyte-bot
Copy link
Contributor

flyte-bot commented Feb 2, 2025

Code Review Agent Run #942e72

Actionable Suggestions - 1
  • flytekit/core/reference_entity.py - 1
    • Consider impact of keyword to positional change · Line 189-189
Review Details
  • Files reviewed - 2 · Commit Range: 88f3f80..6956e28
    • flytekit/core/promise.py
    • flytekit/core/reference_entity.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

Copy link

codecov bot commented Feb 2, 2025

Codecov Report

Attention: Patch coverage is 14.28571% with 6 lines in your changes missing coverage. Please review.

Project coverage is 76.13%. Comparing base (5607b0d) to head (6956e28).

Files with missing lines Patch % Lines
flytekit/core/promise.py 0.00% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3104      +/-   ##
==========================================
- Coverage   79.52%   76.13%   -3.39%     
==========================================
  Files         272      202      -70     
  Lines       24614    21475    -3139     
  Branches     2768     2770       +2     
==========================================
- Hits        19574    16351    -3223     
- Misses       4238     4308      +70     
- Partials      802      816      +14     

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

@flyte-bot
Copy link
Contributor

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
Feature Improvement - Add Positional Arguments Support for Reference Entities

promise.py - Added argument validation and conversion from positional args to kwargs

reference_entity.py - Modified reference entity to support positional arguments and removed args restriction

@@ -187,7 +186,7 @@ def construct_node_metadata(self) -> _workflow_model.NodeMetadata:
return _workflow_model.NodeMetadata(name=extract_obj_name(self.name))

def compile(self, ctx: FlyteContext, *args, **kwargs):
return create_and_link_node(ctx, entity=self, **kwargs)
return create_and_link_node(ctx, self, *args, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider impact of keyword to positional change

Consider if the change from entity=self to positional argument self could impact any callers expecting keyword arguments. The function signature in create_and_link_node expects entity as a keyword parameter.

Code suggestion
Check the AI-generated fix before applying
Suggested change
return create_and_link_node(ctx, self, *args, **kwargs)
return create_and_link_node(ctx, entity=self, *args, **kwargs)

Code Review Run #942e72


Is this a valid issue, or was it incorrectly flagged by the Agent?

  • it was incorrectly flagged

@@ -200,10 +199,6 @@ def __call__(self, *args, **kwargs):
# nothing. Subsequent tasks will have to know how to unwrap these. If by chance a non-Flyte task uses a
# task output as an input, things probably will fail pretty obviously.
# Since this is a reference entity, it still needs to be mocked otherwise an exception will be raised.
if len(args) > 0:
Copy link
Member

Choose a reason for hiding this comment

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

Could we convert args to kwargs here instead? Then, we don't need to change the signature of create_and_link_node

Copy link
Member

Choose a reason for hiding this comment

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

It will be great to have a small test for it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I will modify it and add some tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants