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

fix: new function have no image #356

Merged
merged 8 commits into from
Feb 1, 2024
Merged

fix: new function have no image #356

merged 8 commits into from
Feb 1, 2024

Conversation

ThibaultFy
Copy link
Member

@ThibaultFy ThibaultFy commented Jan 15, 2024 •
edited
Loading

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?

Checklist

  • changelog was updated with notable changes
  • documentation was updated

@ThibaultFy
chore: rm image from newfunction
af8ecfc 
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy
chore: rm get or add function for addressables
e126f97 
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy
Copy link
Member Author

/e2e --tests sdk,substrafl --refs orchestrator=fix/new-function-have-no-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 Jan 16, 2024 •
edited
Loading

End to end tests: ❌ FAILURE

Jobs status:

“Boy, that escalated quickly.” ― Ron Burgundy, Anchorman: The Legend of Ron Burgundy

@ThibaultFy
chore: fix tests
352d6ad 
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy
Copy link
Member Author

/e2e --tests sdk,substrafl --refs orchestrator=fix/new-function-have-no-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 Jan 17, 2024 •
edited
Loading

End to end tests: ❌ FAILURE

Jobs status:

“Success is not final; failure is not fatal: It is the courage to continue that counts.” ―- Winston S. Churchill

@ThibaultFy
Copy link
Member Author

/e2e --tests mnist,substrafl --refs orchestrator=fix/new-function-have-no-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 Jan 17, 2024 •
edited
Loading

End to end tests: ❌ FAILURE

Jobs status:

Epic fail.

@ThibaultFy ThibaultFy changed the base branch from main to feat/add-function-image January 19, 2024 15:03
@ThibaultFy
Copy link
Member Author

/e2e --tests sdk,mnist --refs orchestrator=fix/new-function-have-no-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 Jan 29, 2024 •
edited
Loading

End to end tests: ❌ FAILURE

Jobs status:

It'll stay between us, no one needs to know.

@ThibaultFy
test: update tests
1b3e8a1 
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy
Copy link
Member Author

/e2e --tests sdk,mnist --refs orchestrator=fix/new-function-have-no-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 Jan 29, 2024 •
edited
Loading

End to end tests: ❌ FAILURE

Jobs status:

“You shall not pass!” ― Gandalf, The Lord of the Rings, The Fellowship of the Ring

@ThibaultFy
Copy link
Member Author

/e2e --tests sdk,mnist --refs orchestrator=fix/new-function-have-no-image,substra-backend=feat/check-image-checksum,substra=feat/rename-function-to-archive,substrafl=feat/rename-function-to-archive,substra-ci=revert/frontend-parse-input

@Owlfred
Copy link
Contributor

Owlfred commented Jan 30, 2024 •
edited
Loading

End to end tests: ❌ FAILURE

“Boy, that escalated quickly.” ― Ron Burgundy, Anchorman: The Legend of Ron Burgundy

@ThibaultFy
Copy link
Member Author

/e2e --tests sdk,mnist --refs orchestrator=fix/new-function-have-no-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 Jan 30, 2024 •
edited
Loading

End to end tests: ❌ FAILURE

Jobs status:

Too bad.

@ThibaultFy
Copy link
Member Author

/e2e --tests substrafl,camelyon --refs orchestrator=fix/new-function-have-no-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 Jan 30, 2024 •
edited
Loading

End to end tests: ✔️ SUCCESS

“To infinity and beyond!” ― Buzz Lightyear, Toy Story

@ThibaultFy
update image only if given
e4760ce 
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy
Copy link
Member Author

/e2e --tests sdk,mnist,substrafl,camelyon,doc --refs orchestrator=fix/new-function-have-no-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 Jan 31, 2024 •
edited
Loading

End to end tests: ❌ FAILURE

Jobs status:

It'll stay between us, no one needs to know.

@ThibaultFy ThibaultFy marked this pull request as ready for review January 31, 2024 10:42
@ThibaultFy ThibaultFy requested a review from a team as a code owner January 31, 2024 10:42
SdgJlbl
SdgJlbl reviewed Jan 31, 2024
View reviewed changes
lib/service/function_test.go
Name: "Updated function name",
Key: "4c67ad88-309a-48b4-8bc4-c2e2c1a87a83",
Name: "Updated function name",
Image: &asset.Addressable{StorageAddress: "test/storage/address", Checksum: "4c67ad88309a48b48bc4c2e2c1a87a83"},
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we want to duplicate this test, to check the update of the name / the image, or both?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a test that update name only. But as the name cannot be blank (raise an error), I did't add the test where we update the image only.
So we have one test that updates both, and one test that updates the name only.

@ThibaultFy
chore: return nil outside else
84372d8 
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy
test: duplicate test on update name only
5027181 
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy
Copy link
Member Author

/e2e --tests sdk,mnist,substrafl,camelyon --refs orchestrator=fix/new-function-have-no-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 1, 2024 •
edited
Loading

End to end tests: ❌ FAILURE

“Rien ne sert de courir ; il faut partir à point.” ― Jean de La Fontaine (Le Lièvre et la Tortue)

@ThibaultFy ThibaultFy requested a review from SdgJlbl February 1, 2024 10:28
@ThibaultFy
Copy link
Member Author

/e2e --tests sdk --refs orchestrator=fix/new-function-have-no-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 1, 2024 •
edited
Loading

End to end tests: ❌ FAILURE

“Rien ne sert de courir ; il faut partir à point.” ― Jean de La Fontaine (Le Lièvre et la Tortue)

@ThibaultFy
typo
aa5f8d9 
Signed-off-by: ThibaultFy <thibault.fouqueray@gmail.com>
@ThibaultFy
Copy link
Member Author

/e2e --tests sdk --refs orchestrator=fix/new-function-have-no-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 1, 2024 •
edited
Loading

End to end tests: ✔️ SUCCESS

Awesome! 🎉

@ThibaultFy ThibaultFy merged commit b00ef66 into feat/add-function-image Feb 1, 2024
5 checks passed
@ThibaultFy ThibaultFy deleted the fix/new-function-have-no-image branch February 1, 2024 15:45
ThibaultFy added a commit that referenced this pull request Feb 1, 2024
@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 <thibault.fouqueray@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SdgJlbl SdgJlbl SdgJlbl 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.
3 participants
@ThibaultFy @Owlfred @SdgJlbl