Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: ensure building order #740

Merged
merged 38 commits into from
Feb 12, 2024
Merged

feat: ensure building order #740

merged 38 commits into from
Feb 12, 2024

Conversation

guilhem-barthes
Copy link
Contributor

@guilhem-barthes guilhem-barthes commented Sep 25, 2023 •
edited
Loading

Description

Add some tests on the order the functions are built. Part of FL-1147

How has this been tested?

Checklist

  • changelog was updated with notable changes
  • documentation was updated

@guilhem-barthes guilhem-barthes changed the title Feat/ensure building order feat: ensure building order Sep 25, 2023
@github-actions github-actions bot added documentation Improvements or additions to documentation api compute-engine labels Sep 25, 2023
@guilhem-barthes guilhem-barthes force-pushed the feat/ensure-building-order branch from 74ec241 to 94d5943 Compare September 26, 2023 06:16
@github-actions github-actions bot removed documentation Improvements or additions to documentation api compute-engine labels Sep 26, 2023
@guilhem-barthes guilhem-barthes changed the title feat: ensure building order [sub]feat: ensure building order Sep 26, 2023
@github-actions github-actions bot added the api label Sep 26, 2023
@guilhem-barthes guilhem-barthes force-pushed the feat/ensure-building-order branch from 66a0c7f to ac3a70f Compare October 6, 2023 18:04
@github-actions github-actions bot removed the api label Oct 6, 2023
@guilhem-barthes guilhem-barthes marked this pull request as ready for review October 6, 2023 18:13
@guilhem-barthes guilhem-barthes requested a review from a team as a code owner October 6, 2023 18:13
@guilhem-barthes guilhem-barthes force-pushed the feat/ensure-building-order branch from ac3a70f to 5a02617 Compare October 6, 2023 19:01
@github-actions github-actions bot added documentation Improvements or additions to documentation api compute-engine labels Oct 6, 2023
@linear
Copy link

linear bot commented Oct 6, 2023

FL-1147 Ensure images are built in the right (= rank) order

+ retry with the right prio

@guilhem-barthes guilhem-barthes force-pushed the feat/ensure-building-order branch from 5a02617 to 293b6e1 Compare October 6, 2023 19:11
@github-actions github-actions bot removed documentation Improvements or additions to documentation api compute-engine labels Oct 6, 2023
@guilhem-barthes guilhem-barthes force-pushed the feat/ensure-building-order branch from 795805c to bd03de1 Compare October 6, 2023 19:51
@thbcmlowk thbcmlowk force-pushed the poc-decoupled-builder branch from d52438e to bdb5183 Compare October 11, 2023 15:42
@SdgJlbl SdgJlbl force-pushed the poc-decoupled-builder branch from 96fec22 to 91a431d Compare October 20, 2023 10:22
@thbcmlowk thbcmlowk force-pushed the poc-decoupled-builder branch from b21d5fc to fdfca85 Compare October 24, 2023 09:44
Base automatically changed from poc-decoupled-builder to main October 25, 2023 13:11
@guilhem-barthes guilhem-barthes force-pushed the feat/ensure-building-order branch from bd03de1 to 76815db Compare October 25, 2023 13:35
guilhem-barthes and others added 25 commits February 12, 2024 12:12
@guilhem-barthes
[sub]fix: add missing migration poc (#728)
6f8d6c4 
## 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]>
@guilhem-barthes @SdgJlbl
[sub]feat: add function events (#714)
a8b2398 
- 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]>
@thbcmlowk @guilhem-barthes
[sub]fix(app/orchestrator/resources): FunctionStatus.FUNCTION_STATUS_…
5165a48 
…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]>
@guilhem-barthes
fix: rebase changelog
3a75ba5 
Signed-off-by: Guilhem Barthés <[email protected]>
@SdgJlbl @guilhem-barthes
feat: decouple image builder from worker
847adf3 
Signed-off-by: SdgJlbl <[email protected]>
@guilhem-barthes
feat: add ServiceAccount and modify role
48f8631 
Signed-off-by: Guilhem Barthes <[email protected]>
@guilhem-barthes @SdgJlbl
feat: build function at registration (#707)
3b60001 
<!-- 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]>
@guilhem-barthes
feat: save status update in orc
5edddd1 
Signed-off-by: Guilhem Barthes <[email protected]>
@guilhem-barthes
feat: use status for build waiting
7311a44 
Signed-off-by: Guilhem Barthes <[email protected]>
@guilhem-barthes
fix: re-add container_image_exists
c1a356b 
Signed-off-by: Guilhem Barthes <[email protected]>
@guilhem-barthes
fix: rebase errors
1b6f0c1 
Signed-off-by: Guilhem Barthes <[email protected]>
@guilhem-barthes
fix: format
8822b2b 
Signed-off-by: Guilhem Barthes <[email protected]>
@guilhem-barthes
fix: tests
5df73da 
Signed-off-by: Guilhem Barthes <[email protected]>
@guilhem-barthes
fix: add si to building invokations
e05e3f2 
Signed-off-by: Guilhem Barthes <[email protected]>
@guilhem-barthes
fix: tests
21bd3c4 
Signed-off-by: Guilhem Barthes <[email protected]>
@guilhem-barthes
fix: apply feedback
8ccd4fc 
Signed-off-by: Guilhem Barthes <[email protected]>
@guilhem-barthes
fix: only import during typing
2fc8b03 
Signed-off-by: Guilhem Barthes <[email protected]>
@guilhem-barthes
[sub]feat: modify computetask failure report (#727)
022d93d 
## 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
feat: add config to run celery in tests
a3f9959 
Signed-off-by: Guilhem Barthés <[email protected]>
@guilhem-barthes
feat: add tests
1089dae 
Signed-off-by: Guilhem Barthés <[email protected]>
@guilhem-barthes
fix: remove rebqse duplicate
5370cc2 
Signed-off-by: Guilhem Barthés <[email protected]>
@guilhem-barthes
docs: changelog
28b3339 
Signed-off-by: Guilhem Barthés <[email protected]>
@guilhem-barthes
fix: adapt to pydantic 2.x.x
bd81ffe 
Signed-off-by: Guilhem Barthés <[email protected]>
@guilhem-barthes
fix: remove rebase artifacts
5185562 
Signed-off-by: Guilhem Barthés <[email protected]>
@guilhem-barthes
fix: update to pydantic 2.x.x
1081a3f 
Signed-off-by: Guilhem Barthés <[email protected]>
@guilhem-barthes guilhem-barthes force-pushed the feat/ensure-building-order branch from 5ce6013 to 1081a3f Compare February 12, 2024 11:12
@guilhem-barthes
Copy link
Contributor Author

/e2e

@Owlfred
Copy link

Owlfred commented Feb 12, 2024 •
edited
Loading

End to end tests: ✔️ SUCCESS

“I love it when a plan comes together.” ― Colonel John “Hannibal” Smith, The A-Team

@guilhem-barthes guilhem-barthes merged commit e7aa3e7 into main Feb 12, 2024
6 checks passed
@guilhem-barthes guilhem-barthes deleted the feat/ensure-building-order branch February 12, 2024 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thbcmlowk thbcmlowk thbcmlowk approved these changes

Assignees
No one assigned
Labels
compute-engine
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@guilhem-barthes @Owlfred @thbcmlowk @SdgJlbl @ThibaultFy