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

Pass additional fields to agent create #2272

Conversation

noahjax
Copy link
Contributor

@noahjax noahjax commented Mar 15, 2024

Tracking issue

https://github.com/flyteorg/flyte/issues/

Why are the changes needed?

With the update to flytekit 1.11.0 (specifically this PR) and creation of AsyncAgentBase, output_prefix was removed from the create function for agents. This is problematic for our use case, and in general seems like it is moving things towards less flexible agents.

We would also like to be able to consume the task execution metadata in our create function if possible, and I may tack that on to this PR later.

What changes were proposed in this pull request?

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

@noahjax noahjax force-pushed the noahjax.pass-createtaskrequest-fields-to-agent-create branch 2 times, most recently from ff90b3f to 4f090e6 Compare March 15, 2024 20:40
Copy link

codecov bot commented Mar 15, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 83.36%. Comparing base (3642ec6) to head (38447ae).
Report is 1 commits behind head on master.

Files Patch % Lines
flytekit/extend/backend/agent_service.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2272      +/-   ##
==========================================
- Coverage   83.76%   83.36%   -0.41%     
==========================================
  Files         329      311      -18     
  Lines       25002    24193     -809     
  Branches     3708     3496     -212     
==========================================
- Hits        20943    20168     -775     
+ Misses       3432     3398      -34     
  Partials      627      627              

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

@noahjax noahjax force-pushed the noahjax.pass-createtaskrequest-fields-to-agent-create branch from 4f090e6 to 8c048ac Compare March 15, 2024 22:00
@noahjax noahjax force-pushed the noahjax.pass-createtaskrequest-fields-to-agent-create branch from 43d59de to 059cdb7 Compare March 15, 2024 22:37
@noahjax noahjax marked this pull request as ready for review March 15, 2024 22:59
@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Mar 15, 2024
pingsutw
pingsutw previously approved these changes Mar 17, 2024
@dosubot dosubot bot added the lgtm This PR has been approved by maintainer label Mar 17, 2024
@noahjax noahjax force-pushed the noahjax.pass-createtaskrequest-fields-to-agent-create branch from d87ecf8 to 66f61bd Compare March 17, 2024 23:24
@noahjax noahjax force-pushed the noahjax.pass-createtaskrequest-fields-to-agent-create branch from 66f61bd to cdf11c8 Compare March 18, 2024 13:04
@noahjax
Copy link
Contributor Author

noahjax commented Mar 18, 2024

@pingsutw At some point I'd like to also add TaskExecutionMetadata to agent create (more context here). Should I add that to this PR or address it in a followup?

@noahjax noahjax requested a review from pingsutw March 18, 2024 13:06
@ddl-ebrown
Copy link
Contributor

@pingsutw At some point I'd like to also add TaskExecutionMetadata to agent create (more context here). Should I add that to this PR or address it in a followup?

I would work on landing one thing at a time to make it easier to review / merge

@pingsutw
Copy link
Member

@noahjax yes, you can add that to this PR.

@noahjax
Copy link
Contributor Author

noahjax commented Mar 19, 2024

@pingsutw I'm actually not sure if there is an easy way to add TaskExecutionMetadata here...I don't think I can simply add another arg to AsyncAgentBase without updating all subclasses the way I did for output_prefix. Can we merge this smaller change and followup later on how to add this additional field?

@pingsutw
Copy link
Member

pingsutw commented Mar 19, 2024

Sure, either works for me

@noahjax
Copy link
Contributor Author

noahjax commented Mar 19, 2024

This is what I initially tried for adding TaskExecutionMetadata.

I don't see a way to add it without making a breaking change to AsyncAgentBase.create. Is this ok if I update all of its implementations?

@noahjax
Copy link
Contributor Author

noahjax commented Mar 20, 2024

@pingsutw Everything look ok to you?

@pingsutw
Copy link
Member

LGTM, thank you.

Signed-off-by: Kevin Su <[email protected]>
pingsutw
pingsutw previously approved these changes Mar 20, 2024
Copy link
Contributor

@ddl-ebrown ddl-ebrown left a comment

Choose a reason for hiding this comment

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

Approving based on the prior review from @pingsutw

Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @ddl-ebrown @noahjax

@pingsutw pingsutw merged commit 4522dc9 into flyteorg:master Mar 25, 2024
45 of 47 checks passed
fiedlerNr9 pushed a commit that referenced this pull request Jul 25, 2024
Signed-off-by: noahjax <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Co-authored-by: Kevin Su <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by maintainer size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants