-
Notifications
You must be signed in to change notification settings - Fork 334
Feat: Allow setting k8s labels and annotations in task decorator #3243
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
Conversation
Code Review Agent Run #31ab9eActionable Suggestions - 1
Additional Suggestions - 2
Review 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.
|
flytekit/models/task.py
Outdated
| cache_ignore_input_vars, | ||
| is_eager: bool = False, | ||
| generates_deck: bool = False, | ||
| metadata: typing.Optional["K8sObjectMetadata"] = None, |
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.
The K8sObjectMetadata type is referenced but not defined or imported. This will cause a NameError at runtime when the type hint is evaluated.
Code suggestion
Check the AI-generated fix before applying
@@ -1,2 +1,3 @@
import json as _json
import typing
+from flytekit.models.common import K8sObjectMetadata
Code Review Run #31ab9e
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3243 +/- ##
===========================================
- Coverage 88.13% 52.26% -35.87%
===========================================
Files 105 215 +110
Lines 5165 22471 +17306
Branches 0 2950 +2950
===========================================
+ Hits 4552 11745 +7193
- Misses 613 10036 +9423
- Partials 0 690 +690 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code Review Agent Run #163458Actionable Suggestions - 0Additional Suggestions - 1
Review 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 |
flytekit/models/task.py
Outdated
| cache_ignore_input_vars, | ||
| is_eager: bool = False, | ||
| generates_deck: bool = False, | ||
| metadata: typing.Optional["K8sObjectMetadata"] = None, |
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.
should the naming here be k8s_object_metadata?
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.
Changed.
|
@fg91 can you remind me does flyteidl==1.15.4b0 have what this pr needs? if so want to bump the requirement? if not i can cut another idl beta. (just to get tests passing) |
19a9abc to
fa03793
Compare
Yes, this beta release contains the required change, I bumped the version, thank you. Some other tests are failing with sth seemingly unrelated though 🤔 |
Signed-off-by: Fabio Grätz <[email protected]>
Signed-off-by: Fabio Grätz <[email protected]>
Signed-off-by: Fabio Grätz <[email protected]>
…k metadata Signed-off-by: Fabio Grätz <[email protected]>
Signed-off-by: Fabio Grätz <[email protected]>
…changes Signed-off-by: Fabio Grätz <[email protected]>
Signed-off-by: Fabio Grätz <[email protected]>
4c4173d to
cce6503
Compare
|
I honestly do not love adding |
Result of this discussion but I'm happy to implement it another way! How would you expose it @kumare3 ?
Couldn't the same be said about pod template which is a task arg too? |
|
It can be said about pod_template, by the name Moreover, lets document it saying, labels and annotations are optional parameters, that may not be applied in all cases. |
Ok, will change 👍 Thank you! |
|
I thought a lot more and let's keep what you have. +1 Sorry to keep you waiting. Please document |
…teorg#3243) * Allow settint k8s labels and annotations in task decorator Signed-off-by: Fabio Grätz <[email protected]> * Adapt task models unit test Signed-off-by: Fabio Grätz <[email protected]> * Add python auto container task test Signed-off-by: Fabio Grätz <[email protected]> * Add test that vales passed as label and annotations arg end up in task metadata Signed-off-by: Fabio Grätz <[email protected]> * Cover changes in base_task.py Signed-off-by: Fabio Grätz <[email protected]> * Rename metadata field and pin flyteidl to beta release with metadata changes Signed-off-by: Fabio Grätz <[email protected]> * Pin to > instead of = Signed-off-by: Fabio Grätz <[email protected]> --------- Signed-off-by: Fabio Grätz <[email protected]> Co-authored-by: Fabio Grätz <[email protected]> Signed-off-by: Atharva <[email protected]>
Tracking issue
Closes flyteorg/flyte#6238
Why are the changes needed?
See flyteorg/flyte#6421 and flyteorg/flyte#6238.
What changes were proposed in this pull request?
Add
labelsandannotationsarg to@taskdecorator.How was this patch tested?
Added unit tests. Ran admin and propeller with changes proposed in flyteorg/flyte#6421 and made sure that labels/annotations end up on Pods and CRD objects.
Related PRs
flyteorg/flyte#6421
Summary by Bito
This pull request adds Kubernetes metadata support in the task decorator by introducing 'labels' and 'annotations' parameters, enhancing task management in Kubernetes. It also updates the flyteidl dependency for compatibility and includes unit tests to validate these new features.