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.

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

[sub]feat: refactor image transfer and add tests #759

Closed
wants to merge 32 commits into from

Conversation

ThibaultFy
Copy link
Member

@ThibaultFy ThibaultFy commented Oct 9, 2023

Description

closes FL-1233

SdgJlbl and others added 30 commits October 6, 2023 15:05
Signed-off-by: Guilhem Barthes <[email protected]>
Signed-off-by: Guilhem Barthes <[email protected]>
Signed-off-by: Guilhem Barthes <[email protected]>
Signed-off-by: Guilhem Barthes <[email protected]>
Signed-off-by: Guilhem Barthes <[email protected]>
Signed-off-by: Guilhem Barthes <[email protected]>
<!-- 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]>
Signed-off-by: ThibaultFy <[email protected]>
Signed-off-by: ThibaultFy <[email protected]>
Signed-off-by: ThibaultFy <[email protected]>
## 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]>
- 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]>
…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: builder using builder SA

Signed-off-by: Guilhem Barthés <[email protected]>

* docs: changelog

Signed-off-by: Guilhem Barthés <[email protected]>

---------

Signed-off-by: Guilhem Barthés <[email protected]>
Signed-off-by: Guilhem Barthés <[email protected]>
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy ThibaultFy changed the title [sub-feat] refactor image transfer and add tests [sub]feat: refactor image transfer and add tests Oct 9, 2023
@linear
Copy link

linear bot commented Oct 9, 2023

FL-1233 Refactor docker charon in poc builder

Context

The code of docker charon was copy past to the poc builder. Might need some refactoring.

https://github.com/Substra/substra-backend/tree/poc-decoupled-builder/backend/image_transfer

Specification

  • Match interface to our needs (no CLI, no I/O to file storage)
  • Add unit tests

Acceptance criteria

Only useful feature for our use case is left in the poc builder

@ThibaultFy ThibaultFy marked this pull request as ready for review October 9, 2023 09:15
@ThibaultFy ThibaultFy requested a review from a team as a code owner October 9, 2023 09:15
Signed-off-by: ThibaultFy <[email protected]>
@github-actions github-actions bot added documentation Improvements or additions to documentation api compute-engine labels Oct 12, 2023
@SdgJlbl SdgJlbl mentioned this pull request Oct 20, 2023
2 tasks
bytes_iterator, total_size = repository_dxf.pull_blob(blob.digest, size=True)

# we write the blob directly to the zip file
with tqdm(total=total_size, unit="B", unit_scale=True) as pbar:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove tqdm

Base automatically changed from poc-decoupled-builder to main October 25, 2023 13:11
@ThibaultFy
Copy link
Member Author

closing in favour of #775

@ThibaultFy ThibaultFy closed this Oct 26, 2023
@SdgJlbl SdgJlbl deleted the sub-feat/refactor-image-transfer-on-rebase branch March 14, 2024 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api compute-engine documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants