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

Add task execution metadata to agent create #2282

Conversation

noahjax
Copy link
Contributor

@noahjax noahjax commented Mar 19, 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.

This PR goes a step farther and also adds task execution metadata to agent create, as this information is already readily available in the create task request.

What changes were proposed in this pull request?

Add output_prefix and task_execution_metadata to AsyncAgentBase create in a backwards compatible way that allows existing agents to continue to ignore these fields.

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.add-task-execution-metadata-to-agent-create branch from f8c8761 to 92a580c Compare March 19, 2024 03:07
Copy link

codecov bot commented Mar 19, 2024

Codecov Report

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

Project coverage is 83.53%. Comparing base (4522dc9) to head (ef8979e).
Report is 6 commits behind head on master.

Files Patch % Lines
flytekit/models/task.py 72.72% 9 Missing ⚠️
flytekit/extend/backend/agent_service.py 50.00% 1 Missing ⚠️
flytekit/extend/backend/utils.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2282      +/-   ##
==========================================
- Coverage   83.54%   83.53%   -0.01%     
==========================================
  Files         324      324              
  Lines       24592    24626      +34     
  Branches     3496     3503       +7     
==========================================
+ Hits        20545    20572      +27     
- Misses       3419     3427       +8     
+ Partials      628      627       -1     

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

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.

Hey @noahjax let's discuss here

@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?

Why do we need to update all subclasses as well for TaskExecutionMetadata? did you see any error?

@noahjax
Copy link
Contributor Author

noahjax commented Mar 21, 2024

Yeah, with these changes I see this issue in test_dummy_agent where we test an agent that does not add additional fields to the create function

TypeError: DummyAgent.create() takes 3 positional arguments but 4 were given

Signed-off-by: Kevin Su <[email protected]>
@pingsutw
Copy link
Member

@noahjax, I have a quick fix and have pushed it to your branch, which will keep it backward-compatible
8188db1.

@noahjax
Copy link
Contributor Author

noahjax commented Mar 21, 2024

Thanks @pingsutw, I'll try to clean up the rest of this

Signed-off-by: noahjax <[email protected]>
@noahjax noahjax force-pushed the noahjax.add-task-execution-metadata-to-agent-create branch from ab7303a to 2f790da Compare March 21, 2024 23:44
@noahjax noahjax requested a review from pingsutw March 22, 2024 14:50
@noahjax noahjax marked this pull request as ready for review March 22, 2024 17:14
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Mar 22, 2024
@dosubot dosubot bot added the lgtm This PR has been approved by maintainer label Mar 26, 2024
@pingsutw pingsutw merged commit 133e8d5 into flyteorg:master Mar 26, 2024
45 of 48 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:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants