feat: modify failure report #240
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Sep 7, 2023
guilhem-barthes
requested review from
HamdyD,
jmorel and
a team
as code owners
2 tasks
guilhem-barthes
added a commit
to Substra/substra-backend
that referenced
this pull request
Sep 8, 2023
## Companion PR - Substra/orchestrator#277 - Substra/substra-frontend#240 ## Description The aim is to allow registering failure reports not only for compute task but for other kind of assets (for now, functions which are not building as part of the execution of a compute task) - Modifies `ComputeTaskFailureReport`: - renamed the model to `AssetFailureReport` - renamed field `compute_task_key` to `asset_key` (as we can now have a function key) - added field `asset_type` to provide - Updates protobuf reflecting the previous changes - refactor `download_file` in `PermissionMixin` to provide mroe flexibility (and decouple from DRF) - create new `FailableTask` (Celery task): - centralize the logic to submit asset failure ## How has this been tested? As this is going to be merged on a branch that is going to be merged to a POC branch, we use MNIST as a baseline of a working model. We will deal with failing tests on the POC before merging on main. ## Checklist - [x] [changelog](../CHANGELOG.md) was updated with notable changes - [ ] documentation was updated --------- Signed-off-by: Guilhem Barthes <[email protected]>
guilhem-barthes
added a commit
to Substra/orchestrator
that referenced
this pull request
Sep 8, 2023
## Companion PR - Substra/substra-backend#727 - Substra/substra-frontend#240 ## Description Modify `FailureReport`: - add field `asset_type` containing the kind of asset the failure report connect to - rename `compute_task_key` to `asset_key`, which is a [wire compatible change](https://groups.google.com/g/protobuf/c/hX4Mj0P4N0w) (i.e. does not need to be declared as a new field) ## How has this been tested? As this is going to be merged on a branch that is going to be merged to a POC branch, we use MNIST as a baseline of a working model. We will deal with failing tests on the POC before merging on main. The e2e tests are also broken due to an issue on producing dumps during release, but passed locally. ## Checklist - [x] [changelog](../CHANGELOG.md) was updated with notable changes - [ ] documentation was updated --------- Signed-off-by: Guilhem Barthes <[email protected]>
guilhem-barthes
added a commit
to Substra/orchestrator
that referenced
this pull request
Sep 13, 2023
## Companion PR - Substra/substra-backend#727 - Substra/substra-frontend#240 ## Description Modify `FailureReport`: - add field `asset_type` containing the kind of asset the failure report connect to - rename `compute_task_key` to `asset_key`, which is a [wire compatible change](https://groups.google.com/g/protobuf/c/hX4Mj0P4N0w) (i.e. does not need to be declared as a new field) ## How has this been tested? As this is going to be merged on a branch that is going to be merged to a POC branch, we use MNIST as a baseline of a working model. We will deal with failing tests on the POC before merging on main. The e2e tests are also broken due to an issue on producing dumps during release, but passed locally. ## Checklist - [x] [changelog](../CHANGELOG.md) was updated with notable changes - [ ] documentation was updated --------- Signed-off-by: Guilhem Barthes <[email protected]> Signed-off-by: Guilhem Barthés <[email protected]>
|
Do we need to wait for the companion PRs to be merged into their respective |
Yes, we do indeed |
Milouu
approved these changes
Sep 21, 2023
guilhem-barthes
added a commit
to Substra/substra-backend
that referenced
this pull request
Sep 26, 2023
## Companion PR - Substra/orchestrator#277 - Substra/substra-frontend#240 ## Description The aim is to allow registering failure reports not only for compute task but for other kind of assets (for now, functions which are not building as part of the execution of a compute task) - Modifies `ComputeTaskFailureReport`: - renamed the model to `AssetFailureReport` - renamed field `compute_task_key` to `asset_key` (as we can now have a function key) - added field `asset_type` to provide - Updates protobuf reflecting the previous changes - refactor `download_file` in `PermissionMixin` to provide mroe flexibility (and decouple from DRF) - create new `FailableTask` (Celery task): - centralize the logic to submit asset failure ## How has this been tested? As this is going to be merged on a branch that is going to be merged to a POC branch, we use MNIST as a baseline of a working model. We will deal with failing tests on the POC before merging on main. ## Checklist - [x] [changelog](../CHANGELOG.md) was updated with notable changes - [ ] documentation was updated --------- Signed-off-by: Guilhem Barthes <[email protected]>
guilhem-barthes
added a commit
to Substra/substra-backend
that referenced
this pull request
Oct 6, 2023
## Companion PR - Substra/orchestrator#277 - Substra/substra-frontend#240 ## Description The aim is to allow registering failure reports not only for compute task but for other kind of assets (for now, functions which are not building as part of the execution of a compute task) - Modifies `ComputeTaskFailureReport`: - renamed the model to `AssetFailureReport` - renamed field `compute_task_key` to `asset_key` (as we can now have a function key) - added field `asset_type` to provide - Updates protobuf reflecting the previous changes - refactor `download_file` in `PermissionMixin` to provide mroe flexibility (and decouple from DRF) - create new `FailableTask` (Celery task): - centralize the logic to submit asset failure ## How has this been tested? As this is going to be merged on a branch that is going to be merged to a POC branch, we use MNIST as a baseline of a working model. We will deal with failing tests on the POC before merging on main. ## Checklist - [x] [changelog](../CHANGELOG.md) was updated with notable changes - [ ] documentation was updated --------- Signed-off-by: Guilhem Barthes <[email protected]>
guilhem-barthes
added a commit
to Substra/substra-backend
that referenced
this pull request
Oct 6, 2023
## Companion PR - Substra/orchestrator#277 - Substra/substra-frontend#240 ## Description The aim is to allow registering failure reports not only for compute task but for other kind of assets (for now, functions which are not building as part of the execution of a compute task) - Modifies `ComputeTaskFailureReport`: - renamed the model to `AssetFailureReport` - renamed field `compute_task_key` to `asset_key` (as we can now have a function key) - added field `asset_type` to provide - Updates protobuf reflecting the previous changes - refactor `download_file` in `PermissionMixin` to provide mroe flexibility (and decouple from DRF) - create new `FailableTask` (Celery task): - centralize the logic to submit asset failure ## How has this been tested? As this is going to be merged on a branch that is going to be merged to a POC branch, we use MNIST as a baseline of a working model. We will deal with failing tests on the POC before merging on main. ## Checklist - [x] [changelog](../CHANGELOG.md) was updated with notable changes - [ ] documentation was updated --------- Signed-off-by: Guilhem Barthes <[email protected]>
guilhem-barthes
added a commit
to Substra/substra-backend
that referenced
this pull request
Oct 6, 2023
## Companion PR - Substra/orchestrator#277 - Substra/substra-frontend#240 ## Description The aim is to allow registering failure reports not only for compute task but for other kind of assets (for now, functions which are not building as part of the execution of a compute task) - Modifies `ComputeTaskFailureReport`: - renamed the model to `AssetFailureReport` - renamed field `compute_task_key` to `asset_key` (as we can now have a function key) - added field `asset_type` to provide - Updates protobuf reflecting the previous changes - refactor `download_file` in `PermissionMixin` to provide mroe flexibility (and decouple from DRF) - create new `FailableTask` (Celery task): - centralize the logic to submit asset failure ## How has this been tested? As this is going to be merged on a branch that is going to be merged to a POC branch, we use MNIST as a baseline of a working model. We will deal with failing tests on the POC before merging on main. ## Checklist - [x] [changelog](../CHANGELOG.md) was updated with notable changes - [ ] documentation was updated --------- Signed-off-by: Guilhem Barthes <[email protected]>
guilhem-barthes
added a commit
to Substra/substra-backend
that referenced
this pull request
Oct 6, 2023
## Companion PR - Substra/orchestrator#277 - Substra/substra-frontend#240 ## Description The aim is to allow registering failure reports not only for compute task but for other kind of assets (for now, functions which are not building as part of the execution of a compute task) - Modifies `ComputeTaskFailureReport`: - renamed the model to `AssetFailureReport` - renamed field `compute_task_key` to `asset_key` (as we can now have a function key) - added field `asset_type` to provide - Updates protobuf reflecting the previous changes - refactor `download_file` in `PermissionMixin` to provide mroe flexibility (and decouple from DRF) - create new `FailableTask` (Celery task): - centralize the logic to submit asset failure ## How has this been tested? As this is going to be merged on a branch that is going to be merged to a POC branch, we use MNIST as a baseline of a working model. We will deal with failing tests on the POC before merging on main. ## Checklist - [x] [changelog](../CHANGELOG.md) was updated with notable changes - [ ] documentation was updated --------- Signed-off-by: Guilhem Barthes <[email protected]>
Milouu
force-pushed
the
feat/modify-failure-report
branch
from
77b033e to
dfac745
Compare
thbcmlowk
force-pushed
the
feat/modify-failure-report
branch
from
dfac745 to
73ae061
Compare
This was referenced Oct 12, 2023
Signed-off-by: Guilhem Barthes <[email protected]>
Signed-off-by: Guilhem Barthes <[email protected]> Signed-off-by: SdgJlbl <[email protected]>
SdgJlbl
force-pushed
the
feat/modify-failure-report
branch
from
73ae061 to
e68e858
Compare
SdgJlbl
pushed a commit
to Substra/orchestrator
that referenced
this pull request
Oct 25, 2023
## Details - Substra/substra-backend#714 Add function events, used now we decoupled the building of the function with the execution of the compute task. For that it add a status field on the `Function`. It also includes another PR (merged here), to have functions build logs working again. Fixes FL-1160 As this is going to be merged on a branch that is going to be merged to a POC branch, we use MNIST as a baseline of a working model. We will deal with failing tests on the POC before merging on main. ## Companion PR * orchestrator: #310 * backend: Substra/substra-backend#756 * frontend: Substra/substra-frontend#240 * substra-generator: owkin/substra-generator#131 ## Misc - [x] [changelog](../CHANGELOG.md) was updated with notable changes - [ ] documentation was updated --------- ## Description <!-- Please reference issue if any. --> <!-- Please include a summary of your changes. --> ## How has this been tested? <!-- Please describe the tests that you ran to verify your changes. --> ## Checklist - [ ] [changelog](../CHANGELOG.md) was updated with notable changes - [ ] documentation was updated --------- Signed-off-by: Guilhem Barthes <[email protected]> Signed-off-by: Guilhem Barthés <[email protected]> Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: thbcmlowk <[email protected]> Co-authored-by: Guilhem Barthés <[email protected]> Co-authored-by: guilhem-barthes <[email protected]> Co-authored-by: thbcmlowk <[email protected]>
guilhem-barthes
added a commit
to Substra/substra-backend
that referenced
this pull request
Oct 25, 2023
## Companion PR - Substra/orchestrator#277 - Substra/substra-frontend#240 ## Description The aim is to allow registering failure reports not only for compute task but for other kind of assets (for now, functions which are not building as part of the execution of a compute task) - Modifies `ComputeTaskFailureReport`: - renamed the model to `AssetFailureReport` - renamed field `compute_task_key` to `asset_key` (as we can now have a function key) - added field `asset_type` to provide - Updates protobuf reflecting the previous changes - refactor `download_file` in `PermissionMixin` to provide mroe flexibility (and decouple from DRF) - create new `FailableTask` (Celery task): - centralize the logic to submit asset failure ## How has this been tested? As this is going to be merged on a branch that is going to be merged to a POC branch, we use MNIST as a baseline of a working model. We will deal with failing tests on the POC before merging on main. ## Checklist - [x] [changelog](../CHANGELOG.md) was updated with notable changes - [ ] documentation was updated --------- Signed-off-by: Guilhem Barthes <[email protected]>
guilhem-barthes
added a commit
to Substra/substra-backend
that referenced
this pull request
Feb 8, 2024
## Companion PR - Substra/orchestrator#277 - Substra/substra-frontend#240 ## Description The aim is to allow registering failure reports not only for compute task but for other kind of assets (for now, functions which are not building as part of the execution of a compute task) - Modifies `ComputeTaskFailureReport`: - renamed the model to `AssetFailureReport` - renamed field `compute_task_key` to `asset_key` (as we can now have a function key) - added field `asset_type` to provide - Updates protobuf reflecting the previous changes - refactor `download_file` in `PermissionMixin` to provide mroe flexibility (and decouple from DRF) - create new `FailableTask` (Celery task): - centralize the logic to submit asset failure ## How has this been tested? As this is going to be merged on a branch that is going to be merged to a POC branch, we use MNIST as a baseline of a working model. We will deal with failing tests on the POC before merging on main. ## Checklist - [x] [changelog](../CHANGELOG.md) was updated with notable changes - [ ] documentation was updated --------- Signed-off-by: Guilhem Barthes <[email protected]>
guilhem-barthes
added a commit
to Substra/substra-backend
that referenced
this pull request
Feb 12, 2024
## Companion PR - Substra/orchestrator#277 - Substra/substra-frontend#240 ## Description The aim is to allow registering failure reports not only for compute task but for other kind of assets (for now, functions which are not building as part of the execution of a compute task) - Modifies `ComputeTaskFailureReport`: - renamed the model to `AssetFailureReport` - renamed field `compute_task_key` to `asset_key` (as we can now have a function key) - added field `asset_type` to provide - Updates protobuf reflecting the previous changes - refactor `download_file` in `PermissionMixin` to provide mroe flexibility (and decouple from DRF) - create new `FailableTask` (Celery task): - centralize the logic to submit asset failure ## How has this been tested? As this is going to be merged on a branch that is going to be merged to a POC branch, we use MNIST as a baseline of a working model. We will deal with failing tests on the POC before merging on main. ## Checklist - [x] [changelog](../CHANGELOG.md) was updated with notable changes - [ ] documentation was updated --------- Signed-off-by: Guilhem Barthes <[email protected]>
guilhem-barthes
added a commit
to Substra/substra-backend
that referenced
this pull request
Feb 12, 2024
* feat: decouple image builder from worker Signed-off-by: SdgJlbl <[email protected]> * fix: update skaffold config Signed-off-by: Guilhem Barthes <[email protected]> * feat: add `ServiceAccount` and modify role Signed-off-by: Guilhem Barthes <[email protected]> * feat: build image in new pod Signed-off-by: Guilhem Barthes <[email protected]> * chore: rename `deployment-builder.yaml` to `stateful-builder.yaml` Signed-off-by: Guilhem Barthes <[email protected]> * chore: rename `stateful-builder.yaml` to `statefulset-builder.yaml` Signed-off-by: Guilhem Barthes <[email protected]> * chore: centralize params Signed-off-by: Guilhem Barthes <[email protected]> * feat: create `BuildTask` Signed-off-by: Guilhem Barthes <[email protected]> * feat: move more code to `builder` Signed-off-by: Guilhem Barthes <[email protected]> * fix: remove TaskProfiling as Celery task + save Entrypoint in DB Signed-off-by: SdgJlbl <[email protected]> * feat: build function at registration (#707) <!-- Please reference issue if any. --> <!-- Please include a summary of your changes. --> <!-- Please describe the tests that you ran to verify your changes. --> - [ ] [changelog](../CHANGELOG.md) was updated with notable changes - [ ] documentation was updated --------- Signed-off-by: SdgJlbl <[email protected]> Signed-off-by: Guilhem Barthes <[email protected]> Co-authored-by: SdgJlbl <[email protected]> * feat: share images between backends (#708) Signed-off-by: SdgJlbl <[email protected]> * chore: update helm worklfow Signed-off-by: ThibaultFy <[email protected]> * [sub]fix: add missing migration poc (#728) ## Description Add a migration missing in the poc. This migration alters two things: - modify `ComputeTaskFailureReport.logs` - modify `FunctionImage.file` This migration has been generated automatically with `make migrations` ## How has this been tested? <!-- Please describe the tests that you ran to verify your changes. --> ## Checklist - [ ] [changelog](../CHANGELOG.md) was updated with notable changes - [ ] documentation was updated Signed-off-by: Guilhem Barthes <[email protected]> * [sub]feat: add function events (#714) - Substra/orchestrator#263 Add function events, used now we decoupled the building of the function with the execution of the compute task. For that it add a status field on the Function. It also includes another PR (merged here), to have functions build logs working again. In a future PR, we will change the compute task execution to avoid having to wait_for_function_built in compute_task() Fixes FL-1160 As this is going to be merged on a branch that is going to be merged to a POC branch, we use MNIST as a baseline of a working model. We will deal with failing tests on the POC before merging on main. - [x] [changelog](../CHANGELOG.md) was updated with notable changes - [ ] documentation was updated --------- Signed-off-by: SdgJlbl <[email protected]> Signed-off-by: Guilhem Barthes <[email protected]> Signed-off-by: Guilhem Barthés <[email protected]> Co-authored-by: SdgJlbl <[email protected]> * [sub]fix(app/orchestrator/resources): FunctionStatus.FUNCTION_STATUS_CREATED -> FunctionStatus.FUNCTION_STATUS_WAITING (#742) # Issue Backend FunctionStatus are not aligned with [orchestrator definitions](https://github.com/Substra/orchestrator/blob/poc-decoupled-builder/lib/asset/function.proto#L29-L36). In particular, `FunctionStatus.FUNCTION_STATUS_CREATED` leading to the following error: ```txt ValueError: 'FUNCTION_STATUS_WAITING' is not a valid FunctionStatus ``` ## Description FunctionStatus.FUNCTION_STATUS_CREATED -> FunctionStatus.FUNCTION_STATUS_WAITING ## How has this been tested? Running Camelyon benchmark on [poc-builder-flpc](https://substra.org-1.poc-builder-flpc.cg.owkin.tech/compute_plans/a420306f-5719-412b-ab9c-688b7bed9c70/tasks?page=1&ordering=-rank) environment. ## Checklist - [ ] [changelog](../CHANGELOG.md) was updated with notable changes - [ ] documentation was updated --------- Signed-off-by: Thibault Camalon <[email protected]> * fix: rebase changelog Signed-off-by: Guilhem Barthés <[email protected]> * feat: decouple image builder from worker Signed-off-by: SdgJlbl <[email protected]> * feat: add `ServiceAccount` and modify role Signed-off-by: Guilhem Barthes <[email protected]> * feat: build function at registration (#707) <!-- Please reference issue if any. --> <!-- Please include a summary of your changes. --> <!-- Please describe the tests that you ran to verify your changes. --> - [ ] [changelog](../CHANGELOG.md) was updated with notable changes - [ ] documentation was updated --------- Signed-off-by: SdgJlbl <[email protected]> Signed-off-by: Guilhem Barthes <[email protected]> Co-authored-by: SdgJlbl <[email protected]> * feat: save status update in orc Signed-off-by: Guilhem Barthes <[email protected]> * feat: use status for build waiting Signed-off-by: Guilhem Barthes <[email protected]> * fix: re-add `container_image_exists` Signed-off-by: Guilhem Barthes <[email protected]> * fix: rebase errors Signed-off-by: Guilhem Barthes <[email protected]> * fix: format Signed-off-by: Guilhem Barthes <[email protected]> * fix: tests Signed-off-by: Guilhem Barthes <[email protected]> * fix: add `si` to building invokations Signed-off-by: Guilhem Barthes <[email protected]> * fix: tests Signed-off-by: Guilhem Barthes <[email protected]> * fix: apply feedback Signed-off-by: Guilhem Barthes <[email protected]> * fix: only import during typing Signed-off-by: Guilhem Barthes <[email protected]> * [sub]feat: modify computetask failure report (#727) ## Companion PR - Substra/orchestrator#277 - Substra/substra-frontend#240 ## Description The aim is to allow registering failure reports not only for compute task but for other kind of assets (for now, functions which are not building as part of the execution of a compute task) - Modifies `ComputeTaskFailureReport`: - renamed the model to `AssetFailureReport` - renamed field `compute_task_key` to `asset_key` (as we can now have a function key) - added field `asset_type` to provide - Updates protobuf reflecting the previous changes - refactor `download_file` in `PermissionMixin` to provide mroe flexibility (and decouple from DRF) - create new `FailableTask` (Celery task): - centralize the logic to submit asset failure ## How has this been tested? As this is going to be merged on a branch that is going to be merged to a POC branch, we use MNIST as a baseline of a working model. We will deal with failing tests on the POC before merging on main. ## Checklist - [x] [changelog](../CHANGELOG.md) was updated with notable changes - [ ] documentation was updated --------- Signed-off-by: Guilhem Barthes <[email protected]> * feat: add config to run celery in tests Signed-off-by: Guilhem Barthés <[email protected]> * feat: add tests Signed-off-by: Guilhem Barthés <[email protected]> * fix: remove rebqse duplicate Signed-off-by: Guilhem Barthés <[email protected]> * docs: changelog Signed-off-by: Guilhem Barthés <[email protected]> * fix: adapt to pydantic 2.x.x Signed-off-by: Guilhem Barthés <[email protected]> * fix: remove rebase artifacts Signed-off-by: Guilhem Barthés <[email protected]> * fix: update to pydantic 2.x.x Signed-off-by: Guilhem Barthés <[email protected]> --------- Signed-off-by: SdgJlbl <[email protected]> Signed-off-by: Guilhem Barthes <[email protected]> Signed-off-by: ThibaultFy <[email protected]> Signed-off-by: Guilhem Barthés <[email protected]> Signed-off-by: Thibault Camalon <[email protected]> Co-authored-by: SdgJlbl <[email protected]> Co-authored-by: ThibaultFy <[email protected]> Co-authored-by: Thibault Camalon <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Companion PR
Description
Change the
LogsModalto get a key, allowing to retrieve logs not directly linked with a compute taskHow to test
Screenshots
Notes for developers and reviewers:
Changelog updated