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: add function image #288

Merged
merged 16 commits into from
Feb 8, 2024
Merged

feat: add function image #288

merged 16 commits into from
Feb 8, 2024

Conversation

SdgJlbl
Copy link
Contributor

@SdgJlbl SdgJlbl commented Sep 21, 2023 •
edited
Loading

Description

Add the docker image URI + checksum to the orchestrator

Reviewable commit by commit

Companion PR Substra/substra-backend#739

Fixes FL-1149

How has this been tested?

Checklist

  • changelog was updated with notable changes
  • documentation was updated

@SdgJlbl SdgJlbl force-pushed the feat/add-function-image branch 10 times, most recently from fa578cf to 6c94da8 Compare September 22, 2023 16:23
@SdgJlbl SdgJlbl mentioned this pull request Oct 2, 2023
Merged
4 tasks
@SdgJlbl SdgJlbl force-pushed the feat/add-function-image branch from a298c9b to b69ecc8 Compare October 6, 2023 10:02
@thbcmlowk thbcmlowk force-pushed the poc-decoupled-builder branch 2 times, most recently from 72d3647 to 01e13e7 Compare October 11, 2023 15:06
@SdgJlbl SdgJlbl force-pushed the feat/add-function-image branch from 8bdc2b9 to 3f6f539 Compare October 18, 2023 14:41
@linear
Copy link

linear bot commented Oct 18, 2023

FL-1149 Checking Docker images checksums after download

We bypass the check in https://github.com/Substra/substra-backend/blob/b0442bace3c2de6c3d5936c1101fe63098be3887/backend/substrapp/clients/organization.py#L177 because we don't share the checksum with all backends (through a gRPC event)

@SdgJlbl SdgJlbl force-pushed the feat/add-function-image branch 3 times, most recently from 66d521c to 1cde2ff Compare October 19, 2023 07:43
@SdgJlbl SdgJlbl force-pushed the poc-decoupled-builder branch 3 times, most recently from 04cb078 to 5ccaa18 Compare October 20, 2023 14:14
@thbcmlowk thbcmlowk force-pushed the poc-decoupled-builder branch 2 times, most recently from e93a611 to 0245a55 Compare October 24, 2023 09:37
Base automatically changed from poc-decoupled-builder to main October 25, 2023 13:11
@SdgJlbl SdgJlbl force-pushed the feat/add-function-image branch from 1cde2ff to 8cb67d4 Compare October 27, 2023 08:47
@SdgJlbl SdgJlbl marked this pull request as ready for review December 14, 2023 11:13
@SdgJlbl SdgJlbl requested a review from a team as a code owner December 14, 2023 11:13
@SdgJlbl SdgJlbl force-pushed the feat/add-function-image branch from 79642a3 to bda611a Compare December 14, 2023 11:17
@ThibaultFy ThibaultFy force-pushed the feat/add-function-image branch from c7cbbfa to 5fea929 Compare December 21, 2023 15:54
@ThibaultFy ThibaultFy force-pushed the feat/add-function-image branch from 5fea929 to b758139 Compare January 10, 2024 14:05
@ThibaultFy ThibaultFy mentioned this pull request Jan 19, 2024
Merged
2 tasks
ThibaultFy added a commit that referenced this pull request Feb 1, 2024
@ThibaultFy
fix: new function have no image (#356)
b00ef66 
## Description

On top of #288.

We remove Image from NewFunction, and we create the Addressable Image
with empty strings when we register the Function.
We create the `getOrAddAddressable` to avoid multiple creation of the
empty Image Addressable.

## 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: ThibaultFy <[email protected]>
SdgJlbl and others added 13 commits February 1, 2024 16:46
@SdgJlbl @ThibaultFy
chore: rename the function attribute of Function archive
204fa15 
Signed-off-by: SdgJlbl <[email protected]>
Signed-off-by: ThibaultFy <[email protected]>
@SdgJlbl @ThibaultFy
feat: add Image field to Function object
a3aa669 
Signed-off-by: SdgJlbl <[email protected]>
Signed-off-by: ThibaultFy <[email protected]>
@SdgJlbl @ThibaultFy
chore: add migration
03a9061 
Signed-off-by: SdgJlbl <[email protected]>
Signed-off-by: ThibaultFy <[email protected]>
@SdgJlbl @ThibaultFy
feat: update function image checksum
d4ecd68 
Signed-off-by: SdgJlbl <[email protected]>
Signed-off-by: ThibaultFy <[email protected]>
@SdgJlbl @ThibaultFy
fix: nullable addressable image field
adf36b1 
Signed-off-by: SdgJlbl <[email protected]>
Signed-off-by: ThibaultFy <[email protected]>
@SdgJlbl @ThibaultFy
fix: update Image Addressable
c820e81 
Signed-off-by: SdgJlbl <[email protected]>
Signed-off-by: ThibaultFy <[email protected]>
@SdgJlbl @ThibaultFy
fix tests
87ea93d 
Signed-off-by: SdgJlbl <[email protected]>
Signed-off-by: ThibaultFy <[email protected]>
@SdgJlbl @ThibaultFy
[auto] generate database diagram
a72bb52 
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: SdgJlbl <[email protected]>
Signed-off-by: ThibaultFy <[email protected]>
@SdgJlbl @ThibaultFy
fixup! chore: add migration
49a5fea 
Signed-off-by: SdgJlbl <[email protected]>
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
chore: fix migration
af73cae 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
chore: fix tests
9342759 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
chore: add column empty to addressables
a63421f 
Signed-off-by: ThibaultFy <[email protected]>
@ThibaultFy
fix: new function have no image (#356)
797dfbe 
## Description

On top of #288.

We remove Image from NewFunction, and we create the Addressable Image
with empty strings when we register the Function.
We create the `getOrAddAddressable` to avoid multiple creation of the
empty Image Addressable.

## 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: ThibaultFy <[email protected]>
@ThibaultFy ThibaultFy force-pushed the feat/add-function-image branch from b00ef66 to 797dfbe Compare February 1, 2024 15:46
guilhem-barthes
guilhem-barthes approved these changes Feb 2, 2024
View reviewed changes
Copy link
Contributor

@guilhem-barthes guilhem-barthes left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution ! I don't remember why we decided to not accept Image to be null, but this looks like a decent solution keeping this in mind

server/standalone/dbal/addressable.go Outdated
@@ -14,6 +14,50 @@ func (d *DBAL) addAddressable(addressable *asset.Addressable) error {
return d.exec(stmt)
}

func (d *DBAL) getOrAddAddressable(addressable *asset.Addressable) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the name could be confusing, as we never get the object.

Also, if the intent is to add only if it does not exist, we could consider adding a parameter in addAddressable and when this parameter is set, we could add .Suffix("ON CONFLICT DO NOTHING"). instead of creating this function. This would avoid a transaction to check if the row exists before the transaction for adding it.

Copy link
Member

Choose a reason for hiding this comment

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

I proposed some changes in that sense. Tell me if it seems ok for you :)

@SdgJlbl
Copy link
Contributor Author

SdgJlbl commented Feb 2, 2024

Thanks for your contribution ! I don't remember why we decided to not accept Image to be null, but this looks like a decent solution keeping this in mind

Having a nullable Image requires to alleviate some non-null constraints on the events table, and we thought that it was safer to keep these constraints and create an empty image instead.

@ThibaultFy
Copy link
Member

/e2e --tests sdk --refs orchestrator=feat/add-function-image,substra-backend=feat/check-image-checksum,substra=feat/rename-function-to-archive,substrafl=feat/rename-function-to-archive

@Owlfred
Copy link
Contributor

Owlfred commented Feb 6, 2024 •
edited
Loading

End to end tests: ✔️ SUCCESS

“It’s alive! It’s alive!” ― Henry Frankenstein, Frankenstein

@ThibaultFy
chore: changelog
e6372a2 
Signed-off-by: ThibaultFy <[email protected]>
SdgJlbl
SdgJlbl commented Feb 7, 2024
View reviewed changes
lib/service/function_test.go Outdated Show resolved Hide resolved
server/standalone/dbal/function.go Outdated Show resolved Hide resolved
@ThibaultFy
chore: typo
46b0238 
Signed-off-by: ThibaultFy <[email protected]>
guilhem-barthes
guilhem-barthes approved these changes Feb 8, 2024
View reviewed changes
Copy link
Contributor

@guilhem-barthes guilhem-barthes left a comment

Choose a reason for hiding this comment

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

LGTM 🥇

@ThibaultFy ThibaultFy merged commit e466541 into main Feb 8, 2024
7 checks passed
@ThibaultFy ThibaultFy deleted the feat/add-function-image branch February 8, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThibaultFy ThibaultFy ThibaultFy left review comments

@guilhem-barthes guilhem-barthes guilhem-barthes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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