feat: unify dependencies #155

ThibaultFy · 2023-07-25T14:44:12Z

This reverts commit b9acbe0.
Link to the reverts PR:

Changes regarding the initial commit:

creates the concept of compute dependencies, to build wheels, compile them and create the folder with the right tree structure
dependencies can be computed in a cache folder attached to the Dependency object. This allow to compute the dependencies once and for all using the compute_in_cache_folder and clean_cache_folder method

closes FL-1022, FL-1037,

CI ✅ https://github.com/owkin/substra-ci/actions/runs/5727598535

Please check if the PR fulfills these requirements

If the feature has an impact on the user experience, the changelog has been updated
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)
The commit message follows the conventional commit specification

linear · 2023-08-01T09:09:28Z

FL-1022 Follow-up user-dependencies management

Context

#123 (comment)

Specification

Method copy_local_wheels should not be responsible for building python wheels which would rather be handled beforehand.

Acceptance criteria

copy_local_wheels does not call build_user_dependency_wheel anymore

FL-1037 revert: pip-compile on windows, Windows-compatible temp dir, unify user local dependencies

Summary

Three commits were revert to unlock the release 0.28.0. Hash of the revert commit: b9acbe01b88eb973b6e7c58cf75330bb035873ea

We want to "revert the revert" to re-integrate these changes with the following changes:

Dependencies are compiled 6 times before submitting the CP. As all tasks have the same dependencies, we'd prefer this compilation to be done at object instantiation time (but this will slow down a little subprocess that does not need this compilation to work).
Pinning a lot more versions forces the reinstallation of a lot of dependencies, some of which are already present in the substratools base image. Also, re-installation does not imply deletion, which means that docker images are potentially larger than before. We might want to remove the already installed library from the substratools images (aka use the minimal version of substratools image).
Some tasks fail without logs, and much more recently. The last kaniko pod msg that fails is Taking snapshot of full filesystem... and sometimes triggers a timeout or similar. This symptom is widely described here: Image build process Freezes on Taking snapshot of full filesystem... GoogleContainerTools/kaniko#1333 and is potentially linked to a lack of disk space in ram, but with no real fix or diagnosis for 3 years (last message two weeks ago).

→ Failing example: Product demo Camelyon (cf test upgrade failed run here and the one launched after). To reproduce, launch this example: https://github.com/owkin/product_demo/tree/main/substrafl/camelyon, demo.py file on test-upgrade environment.

We want this PR to be validated on test-upgrade before being merged. To do so, use a compatible version of substrafl and apply this commit.

Notes

Please check if the PR fulfills these requirements

If the feature has an impact on the user experience, the changelog has been updated
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)
The commit message follows the conventional commit specification

SdgJlbl

Thanks a lot for taking up that feature again.
It's a quite diffuse feeling, but the convolutions in some parts of the flow make me wonder if there is not some inversion in the way we are splitting responsibilities (object B should be responsible of doing X, instead of A).
I'd be interested in discussing that further, and will continue thinking about it.

CHANGELOG.md

substrafl/dependency/manage_dependencies.py

substrafl/dependency/schemas.py

substrafl/experiment.py

ThibaultFy · 2023-08-02T14:43:13Z

/e2e --mode=standalone --tests=substrafl --benchmarks=camelyon

Owlfred · 2023-08-02T14:43:16Z

End to end tests: ✔️ SUCCESS

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

substrafl/dependency/manage_dependencies.py

ThibaultFy · 2023-08-16T14:35:21Z

/e2e --mode=standalone --tests=substrafl --benchmarks=camelyon

Owlfred · 2023-08-16T14:35:24Z

End to end tests: ❌ FAILURE

Jobs status:

Camelyon / standalone-camelyon: ❌
Dispatch Jobs: ✔️
Distributed: ⏭️
MNIST: ⏭️
Standalone / standalone-substrafl: ❌

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

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>

ThibaultFy · 2023-08-16T15:40:34Z

/e2e --mode=standalone --tests=substrafl --benchmarks=camelyon

Owlfred · 2023-08-16T15:40:36Z

End to end tests: ✔️ SUCCESS

substrafl/dependency/manage_dependencies.py

substrafl/dependency/schemas.py

SdgJlbl · 2023-08-17T15:47:01Z

substrafl/dependency/schemas.py

+            dest_dir (Path): directory where to copy the dependencies computation tree structure.
+        """
+
+        self._copy_cache_directory(dest_dir=dest_dir)


I don't think we need a method callling another method, you can just have the content of self._copy_cache_directory here, no?

substrafl/dependency/schemas.py

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>

ThibaultFy · 2023-09-01T08:49:27Z

/e2e --mode=standalone --tests=substrafl --benchmarks=camelyon

Owlfred · 2023-09-01T08:49:29Z

End to end tests: ✔️ SUCCESS

“Carpe diem. Seize the day, boys.” ― John Keating, Dead Poets Society

SdgJlbl

Thanks a lot for all your work on this PR.

Just a minor comment to have a more Pythonic getter, and I think we're good to go 🚀

SdgJlbl · 2023-09-01T13:31:15Z

substrafl/dependency/schemas.py

+
+    def get_cache_directory(self) -> Path:


Suggested change

def get_cache_directory(self) -> Path:

@property

def cache_directory(self) -> Path:

oh thanks of course...

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>

ThibaultFy force-pushed the feat/compile-dependencies branch from 225c580 to 88f3ba3 Compare July 25, 2023 15:01

ThibaultFy changed the base branch from main to chore/rename-generate-wheel-to-manage-dep July 25, 2023 15:01

ThibaultFy force-pushed the feat/compile-dependencies branch 2 times, most recently from 490d192 to 252a05b Compare July 26, 2023 08:25

ThibaultFy force-pushed the chore/rename-generate-wheel-to-manage-dep branch from 64ab47d to 999d8f8 Compare July 28, 2023 07:57

Base automatically changed from chore/rename-generate-wheel-to-manage-dep to main July 28, 2023 08:07

ThibaultFy force-pushed the feat/compile-dependencies branch 3 times, most recently from 1c4f1e8 to 532594a Compare July 31, 2023 10:14

ThibaultFy force-pushed the feat/compile-dependencies branch from 656c5c3 to 6a0a976 Compare August 1, 2023 14:33

ThibaultFy changed the title ~~feat: compile dependencies~~ feat: unify dependencies Aug 1, 2023

ThibaultFy force-pushed the feat/compile-dependencies branch 3 times, most recently from 663a973 to 49d0b12 Compare August 1, 2023 14:54

ThibaultFy marked this pull request as ready for review August 1, 2023 14:57

ThibaultFy requested a review from a team as a code owner August 1, 2023 14:57

SdgJlbl reviewed Aug 2, 2023

View reviewed changes

ThibaultFy force-pushed the feat/compile-dependencies branch 3 times, most recently from dbbb3a2 to 9a75fdd Compare August 2, 2023 14:42

ThibaultFy commented Aug 2, 2023

View reviewed changes

substrafl/dependency/manage_dependencies.py Outdated Show resolved Hide resolved

ThibaultFy force-pushed the feat/compile-dependencies branch 2 times, most recently from d6a63a7 to cefd037 Compare August 2, 2023 15:05

ThibaultFy requested a review from SdgJlbl August 2, 2023 15:54

ThibaultFy force-pushed the feat/compile-dependencies branch 2 times, most recently from 4ec6cdf to f4b3325 Compare August 16, 2023 14:34

ThibaultFy force-pushed the feat/compile-dependencies branch 2 times, most recently from b6fa09f to 6d04f0d Compare August 16, 2023 15:07

feat: unify dependencies

1bcd014

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>

ThibaultFy force-pushed the feat/compile-dependencies branch from 6d04f0d to 1bcd014 Compare August 16, 2023 15:36

SdgJlbl reviewed Aug 17, 2023

View reviewed changes

ThibaultFy added 2 commits September 1, 2023 10:35

chore: sarah review

f6ff02b

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>

chore: sarah review

04e4b60

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>

ThibaultFy requested a review from SdgJlbl September 1, 2023 09:54

SdgJlbl approved these changes Sep 1, 2023

View reviewed changes

ThibaultFy added 2 commits September 1, 2023 15:38

chore: change getter to property

d847a16

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>

chore: changelog

94bfa9f

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>

ThibaultFy merged commit 11541b5 into main Sep 1, 2023
6 checks passed

ThibaultFy deleted the feat/compile-dependencies branch September 1, 2023 14:00

Milouu mentioned this pull request Sep 5, 2023

chore: release 0.40.0 #164

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: unify dependencies #155

feat: unify dependencies #155

ThibaultFy commented Jul 25, 2023 •

edited

Loading

linear bot commented Aug 1, 2023

Context

Specification

Acceptance criteria

Summary

Notes

Please check if the PR fulfills these requirements

SdgJlbl left a comment

ThibaultFy commented Aug 2, 2023

Owlfred commented Aug 2, 2023 •

edited

Loading

ThibaultFy commented Aug 16, 2023

Owlfred commented Aug 16, 2023 •

edited

Loading

ThibaultFy commented Aug 16, 2023

Owlfred commented Aug 16, 2023 •

edited

Loading

SdgJlbl Aug 17, 2023

ThibaultFy commented Sep 1, 2023

Owlfred commented Sep 1, 2023 •

edited

Loading

SdgJlbl left a comment

SdgJlbl Sep 1, 2023

ThibaultFy Sep 1, 2023

-    def get_cache_directory(self) -> Path:
+    @property
+    def cache_directory(self) -> Path:

feat: unify dependencies #155

feat: unify dependencies #155

Conversation

ThibaultFy commented Jul 25, 2023 • edited Loading

Please check if the PR fulfills these requirements

linear bot commented Aug 1, 2023

Context

Specification

Acceptance criteria

Summary

Notes

Please check if the PR fulfills these requirements

SdgJlbl left a comment

Choose a reason for hiding this comment

ThibaultFy commented Aug 2, 2023

Owlfred commented Aug 2, 2023 • edited Loading

ThibaultFy commented Aug 16, 2023

Owlfred commented Aug 16, 2023 • edited Loading

ThibaultFy commented Aug 16, 2023

Owlfred commented Aug 16, 2023 • edited Loading

SdgJlbl Aug 17, 2023

Choose a reason for hiding this comment

ThibaultFy commented Sep 1, 2023

Owlfred commented Sep 1, 2023 • edited Loading

SdgJlbl left a comment

Choose a reason for hiding this comment

SdgJlbl Sep 1, 2023

Choose a reason for hiding this comment

ThibaultFy Sep 1, 2023

Choose a reason for hiding this comment

ThibaultFy commented Jul 25, 2023 •

edited

Loading

Owlfred commented Aug 2, 2023 •

edited

Loading

Owlfred commented Aug 16, 2023 •

edited

Loading

Owlfred commented Aug 16, 2023 •

edited

Loading

Owlfred commented Sep 1, 2023 •

edited

Loading