-
Notifications
You must be signed in to change notification settings - Fork 322
Add ability to specify default resource requests/limits for tasks via pyflyte run
and pyflyte register
#3229
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 ability to specify default resource requests/limits for tasks via pyflyte run
and pyflyte register
#3229
Conversation
Code Review Agent Run Status
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3229 +/- ##
==========================================
+ Coverage 83.58% 87.67% +4.08%
==========================================
Files 3 88 +85
Lines 195 4267 +4072
==========================================
+ Hits 163 3741 +3578
- Misses 32 526 +494 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code Review Agent Run Status
|
Code Review Agent Run #85e95aActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
Changelist by BitoThis pull request implements the following key changes.
|
Code Review Agent Run #f176edActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
4bb75b7
to
8291b77
Compare
Signed-off-by: redartera <[email protected]>
8291b77
to
59d0aec
Compare
Code Review Agent Run #a4a15bActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
Code Review Agent Run Status
|
Code Review Agent Run Status
|
Code Review Agent Run Status
|
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.
Thanks, it looks great. I think it's useful, and I'd like to get other reviewers' opinions as well
Signed-off-by: redartera <[email protected]>
Code Review Agent Run #7dbdebActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
Code Review Agent Run Status
|
Signed-off-by: redartera <[email protected]>
Code Review Agent Run Status
|
Hi @redartera , Could we use separate option instead? Like: ❯ pyflyte run file.py --cpu=1 --mem=2Gi ❯ pyflyte run file.py --cpu-limit=1 --mem-limit=2Gi This has follow benefits:
|
Makes sense @machichima - how about just Also, this would probably require we remove |
Bito Review Skipped - No Changes Detected |
Hi @redartera
I still prefer separating request and limit to two, like what majority of CLI tools did (e.g. kubectl), which provide a more explicit way of setting.
It's unfortunate that --default-resources cpu=1 mem=2Gi gpu=1 --limit-resources cpu=2 mem=4Gi
--default-resources cpu=1 mem=2Gi --limit-resources cpu=2
I think it's fine as long as we document it well |
Signed-off-by: redartera <[email protected]>
Signed-off-by: redartera <[email protected]>
@machichima I separated requests and limits into two options as recommended, PTAL. I chose |
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.
Just did a quick walk through. Overall looks good, just some nit.
Will look into detail and try it out later
Signed-off-by: redartera <[email protected]>
8d76fd5
to
a5ca772
Compare
Signed-off-by: redartera <[email protected]>
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.
Thank you for the complete tests!!
Generally, my comment is about putting the resource settings into a variable to use when setting and asserting, making it easier if we want to extend or modify the test in the future
Signed-off-by: redartera <[email protected]>
@machichima PTAL at 968a041 |
Hi @redartera , |
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.
LGTM. Thanks!!
cc @pingsutw |
Why are the changes needed?
When registering tasks with Flyte, users often need flexibility to specify resource requirements at registration time without modifying code. This is particularly useful in scenarios where:
Currently, the only options are to specify resources explicitly in task definitions or rely on cluster-level defaults, which lacks flexibility for selective overrides.
What changes were proposed in this pull request?
This PR introduces the ability to specify default resource requests and limits for tasks that don't have resources explicitly defined. The implementation:
default_resources
parameter toSerializationSettings
that, if set, applies to tasks with unspecified resources--resource-requests
and--resource-limits
for bothpyflyte run
andpyflyte register
commandsresource_callback
) that handles comma-separated key-value pairsExample CLI usage:
How was this patch tested?
Added comprehensive unit and integration tests:
tests/flytekit/unit/interaction/test_click_types.py
tests/flytekit/integration/remote/test_remote.py
for both:pyflyte register
with resource overridespyflyte run --remote
with resource overridesThe integration tests verify end-to-end functionality by:
Check all the applicable boxes
Summary by Bito
This pull request enhances the Flyte framework by allowing users to specify default resource requests and limits for tasks during registration and execution. It introduces new CLI options for `pyflyte run` and `pyflyte register`, along with improved help text for clarity. Comprehensive tests ensure the new functionality operates correctly and handles errors effectively.