Could someone take a look at this PR? It adds cust...
# dev-metaflowe
elegant-beach-10818
06/09/2023, 4:09 PMCould someone take a look at this PR? It adds custom annotation functionality and should be a straightforward review (it uses essentially the same mechanism as kubernetes labels)
https://github.com/Netflix/metaflow/pull/1442
👀 1
elegant-beach-10818
06/19/2023, 7:22 PM@bulky-afternoon-92433 When you have a moment could you take a look at this please?
👌 1
b
bulky-afternoon-92433
06/20/2023, 9:42 AMlooks good to go regarding the Kubernetes part. Had a comment regarding if we want to support custom annotations on Argo workflows as well, as its currently not implemented.
e
elegant-beach-10818
06/21/2023, 4:23 AMGreat point, I implemented the argo workflow annotations as well. Ready for another review
b
bulky-afternoon-92433
06/22/2023, 9:14 AMwent through the PR and the Argo part is looking good as well 👌 The only addition would be to add the same annotations to Sensors as well, so annotations are in sync for all the Argo resources we create. Should be ready to ship after that.
e
elegant-beach-10818
06/26/2023, 6:55 PMAdded the annotations for the Sensors and addressed some of your comments. Let me know what you think
b
bulky-afternoon-92433
06/28/2023, 1:24 PMsensor annotations working great. Two comments, one of them warranting a small change still, but otherwise this is looking ready to ship 🙂
e
elegant-beach-10818
06/28/2023, 4:23 PMMade that small change and decided to go with the original approach for the annotations function. Thanks!
elegant-beach-10818
07/10/2023, 1:54 PM@bulky-afternoon-92433 Could you review my latest changes when you’re available?
👌 1
b
bulky-afternoon-92433
07/10/2023, 8:51 PMI'll take a more thorough peek first thing tomorrow, though I think last I saw it was ready feature-wise 🙂
I've been working on a slight refactor for the Kubernetes labels support which will consolidate the labeling logic to the decorator as much as possible instead of spreading it around components: https://github.com/Netflix/metaflow/pull/1471 if you want to take a peek at the planned changes
A similar pattern for the annotations would be preferable, but I see little reason to delay the current PR due to a refactor. There is one behavior with the label/annotation validation happening outside the decorator, which leads to the job having enough time in the lifecycle to get submitted and launch before validators are run.
bulky-afternoon-92433
07/14/2023, 12:01 PM@elegant-beach-10818 I approved your original PR due to the feature working as intended. As I mentioned earlier, there's a drive to refactor this kind of logic into the Kubernetes decorator instead so a refactor for the annotations would be in place in any case.
I opened a PR on top of your branch https://github.com/tylerpotts/metaflow/pull/1 to incorporate the Kubernetes labels refactoring, as these are somewhat dependant on each other. It also refactors the annotation support into the decorator. If you could eyeball the relevant changes (sorry for the big diff due to different bases) and if they make sense to you, feel free to merge into your branch so we can get this going.
e
elegant-beach-10818
07/14/2023, 2:42 PMreviewed and it looks good. It’s merged now
👌 1
2 Views