-
Notifications
You must be signed in to change notification settings - Fork 443
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
Modify Labels for Katib Components #1611
Modify Labels for Katib Components #1611
Conversation
Looks good, but I don't have enough familiarity to know if the presubmit failure is related. |
/retest |
@@ -142,9 +142,9 @@ const ( | |||
AnnotationIstioSidecarInjectValue = "false" | |||
|
|||
// LabelTrialTemplateConfigMapName is the label name for the Trial templates configMap | |||
LabelTrialTemplateConfigMapName = "app" | |||
LabelTrialTemplateConfigMapName = "katib.kubeflow.org/component" |
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 we use katib.kubeflow.org or automl.kubeflow.org?
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.
I am fine with both, it's better to take what is easier for user to see the Katib components.
WDYT @johnugeorge @alculquicondor @kubeflow/wg-training-leads ?
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.
Are there other projects that would potentially use automl.kubeflow.org/component
?
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.
Currently, we have only Katib project in Kubeflow for AutoML
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.
I think the "component" part might be exclusive to katib.
Of course, you could do automl.kubeflow.org/component: katib-ui
, but maybe it's overkill.
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.
I agree, so in the future if we have more projects for AutoML we can extend it.
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.
As we decided on the WG Meeting we will use katib.kubeflow.org
for now since it is easier to indicate project for the user.
@gaocegege @alculquicondor @johnugeorge if you are fine with the changes, please leave your lgtm
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
/retest |
sidetopic: That's quite a bit of retests. Is there any understanding of the reasons for the flakiness? |
There is a problem with Docker pull limitation. We are getting this error in the CI:
|
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreyvelich, terrytangyuan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
See discussion: kubeflow/common#148 (comment).
I modified labels for Katib components to follow Kubernetes guidelines.
Probably, we should also think about renaming
katib-metricscollector-injection: enabled
tokatib.kubeflow.org/metricscollector-injection: true
/cc @gaocegege @johnugeorge @alculquicondor