-
Notifications
You must be signed in to change notification settings - Fork 241
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
[FrontendSpec] Add internal_labels to frontend_spec #5542
Conversation
mlrun/common/constants.py
Outdated
MLRUN_INTERNAL_LABELS = [ | ||
"dask.org/cluster-name", | ||
"dask.org/component", | ||
"host", | ||
"job-type", | ||
"kind", | ||
"mlrun-auth-keynuclio.io/project-name", | ||
"mlrun/class", | ||
"mlrun/client_python_version", | ||
"mlrun/client_version", | ||
"mlrun/function", | ||
"mlrun/job", | ||
"mlrun/name", | ||
"mlrun/owner", | ||
"mlrun/owner_domain", | ||
"mlrun/project", | ||
"mlrun/runner-pod", | ||
"mlrun/schedule-name", | ||
"mlrun/scrape-metrics", | ||
"mlrun/tag", | ||
"mlrun/uid", | ||
"mlrun/username", | ||
"mlrun/username_domain", | ||
"owner", | ||
"resource_name", | ||
"v3io_user", | ||
"workflow", | ||
] |
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.
This is not helpful to understand each label usage and does not link to its usages - thus, prone to errors (such as label changes, etc).
What I do suggest is
- make it a class, where each subclass is the resource name (e.g. "functions", "runs", "artifacts", ...)
- values would be named so it could be reused - DRY
e.g.
class InternalLabels:
class Runs:
hostname = "hostname"
and so on.
also. note that this code should be used from mlrun server mostly. because mlrun client should not use/enforce internal labels.
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.
About the subclasses, I think we can do it in the following PR
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.
Seems there was a mis-communication regarding my 1.9.0 deprecation comment left some clarifications.
Otherwise great job!
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.
Few more changes are required and some pieces you have missed
MlrunInternalLabels.project: self.project, | ||
MlrunInternalLabels.scrape_metrics: "True", | ||
MlrunInternalLabels.tag: "latest", | ||
MlrunInternalLabels.uid: self.run_uid, | ||
"mpi-job-name": "trainer-1b019005", |
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.
^
MlrunInternalLabels.project: self.project, | ||
MlrunInternalLabels.schedule_name: "True", | ||
MlrunInternalLabels.tag: "latest", | ||
MlrunInternalLabels.uid: self.run_uid, | ||
"mpi-job-name": "trainer-1b019005", |
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.
^
"spark-app-selector": "spark-12f88a73cb544ce298deba34947226a4", | ||
"spark-exec-id": "1", | ||
"spark-role": "executor", |
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.
^
"spark-app-selector": "spark-12f88a73cb544ce298deba34947226a4", | ||
"spark-role": "driver", | ||
"sparkoperator.k8s.io/app-name": "my-spark-jdbc-2ea432f1", |
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.
^
"kind": "job", | ||
MlrunInternalLabels.v3io_user: self._test_env["V3IO_USERNAME"], | ||
MlrunInternalLabels.owner: self._test_env["V3IO_USERNAME"], | ||
MlrunInternalLabels.kind: "job", | ||
"category": "tests", |
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.
^
@liranbg , Pay attention that the over labels you asked me to add are only sets in tests and are not mlrun internal labels. |
Add internal_labels to frontend_spec so the ui can know which labels are internal and should not be editable
After merging this pr we need to keep in mind adding any new internal labels to the
MlrunInternalLabels
class and use it in the code.https://iguazio.atlassian.net/browse/ML-5939