-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat!: multiple performance per task #197
Conversation
9adf1c4
to
3fdda70
Compare
45a74de
to
38f93b2
Compare
MetricKey: "metricKey", | ||
ComputeTaskKey: "taskKey", | ||
MetricKey: "metricKey", | ||
ComputeTaskOutputIdentifier: "performance", |
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.
why is a ComputeTaskOutputIdentifier
named performance
?
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.
It is just for the test. Before this PR, the identifier of a test task was always "performance" even if its a variable. But we could change the name if we want
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 feel like it would be more clear if something different was used, to show that we don't have to use the reserved keyword "performance". But that's a subjective opinion.
PerformanceValue: 3.14, | ||
ComputeTaskKey: "taskUuid", | ||
MetricKey: "metricUuid", | ||
ComputeTaskOutputIdentifier: "performance", |
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.
same as above, wouldn't something like outputIdentifier
be more clear?
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.
It's just an example of identifier
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 but probably worth waiting for the approval of someone who hasn't started learning Go yesterday ^^
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
c1017a0
to
e2ab397
Compare
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
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
## [0.34.0](https://github.com/Substra/orchestrator/releases/tag/0.34.0) - 2023-05-11 ### Changed - A Performance in now unique regarding a compute task key, a metric key and a compute task output identifier ([#197](#197)) ### Removed - Metric from Performance ([#213](#213)) Signed-off-by: Milouu <milan.roustan@owkin.com>
Description
Performance asset were unique regarding their compute task key, and the function key they were computed from. Or, we want to have the possibility to have a function that outputs several performance and store them in the same task.
This implies two main things:
The PR batch modify both the orchestrator and backend db in order to include the identifier as an additional unique condition.
In the backend, we use the ComputeTaskOutput as primary key directly.
Companion PR
E2E tests
https://github.com/owkin/substra-ci/actions/runs/4872054802