feat!: unify user local dependencies #123

SdgJlbl · 2023-05-19T07:39:50Z

Summary

This PR improves the management of user-provided dependencies.

it builds wheels for all user-defined Python modules;
it compiles a unified list of all third-party dependencies (including indirect dependencies) using pip-compile at generation time; this allows for early warning in case of incompatibilities;
it checks at registration time if the Python version is compatible with Substra, else raises early;
renaming of argument for clarity (local_dependencies -> local_installable_dependencies).

Notes

resolves FL-984
resolves FL-995
resolves FL-978
resolves FL-641
resolves FL-642

Still TODO

check docstrings
check for missing unit tests

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-06-05T13:08:09Z

FL-641 Improve the way we handle local dependencies

Context and user need:
See https://www.notion.so/owkin-fdn/Lipome-upgrade-to-0-26-1-809c8fc1b1cd4dee9d2dd2221c18f61a?pvs=4#d7f27449ae6d4ce78e08a1c0bc5205b3

We want to install all the local dep AND the dependencies on the same line to take advantage of the pip compatibility resolver, and to install a lib ONLY ONE TIME.

Functional spec:
We want to install all dep only one time, and have a sanity check of dep compatibility locally and not on the platform.

Technical spec:
Build the wheel locally for local dep (without installing).
Add a local check about dependencies (local + normale) compatibility.
Pass the wheel only in the docker container.
Install all in the same line (local dep + dep).

Acceptance criteria:

FL-995 Rename local_dependencies to local_installable_dependencies

There is often the confusion between local_code (for simple folder) and local_dependencies (for “things” that can be pip installed”). I am not an expert on this but could we find better name?

Let's rename local_dependencies to local_installable_dependencies

FL-984 Installing substrafl and substra from PyPI in Docker

We are currently building a wheel for substraFL and substra during the Docker image creation, even if the user doesn't want to install its local (edited) version, but versions released from PyPI.

I guess it's a left-over from when substra was closed source, and it was easier to do it that way than to handle credentials to install from private PyPI. It's not the case anymore, so we should treat these dependencies as regular PyPI dependencies.

https://github.com/Substra/substrafl/blob/main/substrafl/remote/register/generate_wheel.py#L94

FL-642 Unify third parties dependencies in substrafl

Context and user need:
The same dependency can be installed several times (with a different version pinned) in the Docker image generated by substrafl: one because it's specified explicitely in a Dependency object, and then as indirect dependencies of local dependencies.
This leads to slow build time and large size for Docker images.
In addition, it can be confusing for the user to ask for a given version of Pytorch in their script, and actually get another because another version is pinned as a dependency of a dependency.

Functional spec:

Technical spec:
Good luck

Acceptance criteria:
Dependencies should be installed at most once.

FL-978 Check Python version in Substra

Currently, if we run substra in an Python version not supported (i.e. to which we haven't build substra-tools images), the process will continue until it has to pull the non-existent image.

Th scope of this task is add a check at the beginning of dockerfile creation to not generate Dockerfile with incorrect python versions and stop the process early.

User issue: https://substra-workspace.slack.com/archives/C041LHMR37G/p1684931669897749

ThibaultFy

Impressive PR, thanks a lot !! I just got a bit confused in the end: is there still a substrafl internal folder ? or all wheel are now at the root of the docker ?

CHANGELOG.md

substrafl/remote/register/generate_wheel.py

substrafl/remote/register/manage_dependencies.py

substrafl/remote/register/register.py

tests/remote/register/test_register.py

tests/remote/register/test_manage_dependencies.py

substrafl/remote/register/manage_dependencies.py

substrafl/remote/register/register.py

substrafl/dependency/schemas.py

substrafl/remote/register/manage_dependencies.py

substrafl/remote/register/register.py

ThibaultFy · 2023-06-08T08:39:54Z

/e2e --tests substrafl --benchmarks camelyon

Owlfred · 2023-06-08T08:39:56Z

End to end tests: ✔️ SUCCESS

ThibaultFy

One question and will be good to me :) Really nice work.
And of course deal with the Sphynx issue 😭

tests/remote/register/test_manage_dependencies.py

substrafl/remote/register/manage_dependencies.py

Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>

SdgJlbl · 2023-06-08T16:45:29Z

/e2e --tests substrafl --benchmarks camelyon

Owlfred · 2023-06-08T16:45:32Z

End to end tests: ❌ FAILURE

“Houston, we have a problem.” ― Jim Lovell, Apollo 13

Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>

SdgJlbl · 2023-06-08T17:49:19Z

/e2e --tests substrafl --benchmarks camelyon

Owlfred · 2023-06-08T17:49:20Z

End to end tests: ✔️ SUCCESS

Awesome! 🎉

ThibaultFy · 2023-06-09T08:58:34Z

/e2e test None --benchmarks camelyon

Owlfred · 2023-06-09T08:58:38Z

End to end tests: ❌ FAILURE

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

SdgJlbl · 2023-06-09T09:09:26Z

@ThibaultFy rerun the benchmark that failed this night because of a network error, all good now https://github.com/owkin/substra-ci/actions/runs/5214219498/jobs/9421588382 ✅

ThibaultFy · 2023-06-09T09:09:53Z

Oh perfect thanks !

ThibaultFy

Really nice work, can't wait to see the impact 😄

ThibaultFy · 2023-06-09T09:10:37Z

docs/conf.py

@@ -193,6 +193,7 @@
    ("py:class", "substra.sdk.schemas.FunctionOutputSpec"),
    ("py:class", "substra.sdk.schemas.FunctionInputSpec"),
    ("py:class", "ComputePlan"),
+    ("py:class", "Path"),


🤣 you gave up...

substrafl/remote/register/manage_dependencies.py

Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>

This reverts commit 157cf59.

This reverts commit 157cf59. Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>

SdgJlbl force-pushed the feat/unify-user-dependencies branch 2 times, most recently from 0dbb716 to 66af593 Compare May 19, 2023 16:19

SdgJlbl changed the title ~~Feat/unify user dependencies~~ Feat: unify user local dependencies May 19, 2023

Base automatically changed from feat/add-dockerignore to main May 29, 2023 08:43

SdgJlbl force-pushed the feat/unify-user-dependencies branch 4 times, most recently from 0195cbf to 41e9009 Compare June 5, 2023 12:58

SdgJlbl force-pushed the feat/unify-user-dependencies branch from 41e9009 to 4bb8669 Compare June 5, 2023 13:09

SdgJlbl marked this pull request as ready for review June 5, 2023 13:10

SdgJlbl requested a review from a team as a code owner June 5, 2023 13:10

ThibaultFy changed the title ~~Feat: unify user local dependencies~~ feat: unify user local dependencies Jun 5, 2023

ThibaultFy reviewed Jun 5, 2023

View reviewed changes

SdgJlbl force-pushed the feat/unify-user-dependencies branch from 4bb8669 to c8dafc2 Compare June 7, 2023 07:28

SdgJlbl requested a review from ThibaultFy June 7, 2023 07:30

SdgJlbl force-pushed the feat/unify-user-dependencies branch from c8dafc2 to d0c2d05 Compare June 7, 2023 07:50

guilhem-barthes reviewed Jun 7, 2023

View reviewed changes

substrafl/remote/register/register.py Outdated Show resolved Hide resolved

SdgJlbl force-pushed the feat/unify-user-dependencies branch from d0c2d05 to 181d08b Compare June 7, 2023 14:35

SdgJlbl changed the title ~~feat: unify user local dependencies~~ feat!: unify user local dependencies Jun 7, 2023

SdgJlbl force-pushed the feat/unify-user-dependencies branch from 181d08b to ccbd113 Compare June 7, 2023 15:02

ThibaultFy reviewed Jun 8, 2023

View reviewed changes

tests/remote/register/test_manage_dependencies.py Outdated Show resolved Hide resolved

substrafl/remote/register/manage_dependencies.py Show resolved Hide resolved

SdgJlbl added 5 commits June 8, 2023 18:44

feat: unify the handling of user-provided local dependencies

b818819

Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>

Rename generate_wheel to manage_dependencies

25258ce

Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>

Add check for Python version

6ba7cd8

Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>

Changelog + linter

c08e666

Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>

Add error managements to wheel building and pip compiling

e2663e7

Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>

SdgJlbl added 4 commits June 8, 2023 18:44

Fix Dependency object docstring

6dfe677

Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>

Various fixes

a1ead3b

Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>

Rename local_dependencies to local_installable_dependencies

e07b75b

Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>

PR comments

db48232

Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>

SdgJlbl force-pushed the feat/unify-user-dependencies branch from 72e9f87 to d786742 Compare June 8, 2023 16:45

SdgJlbl force-pushed the feat/unify-user-dependencies branch from d786742 to 3e16f7e Compare June 8, 2023 17:04

Unit tests

f4a9e4d

Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>

SdgJlbl force-pushed the feat/unify-user-dependencies branch from 3e16f7e to f4a9e4d Compare June 8, 2023 17:37

SdgJlbl requested a review from ThibaultFy June 8, 2023 17:52

ThibaultFy approved these changes Jun 9, 2023

View reviewed changes

thbcmlowk reviewed Jun 9, 2023

View reviewed changes

SdgJlbl force-pushed the feat/unify-user-dependencies branch from f452359 to e110bdd Compare June 9, 2023 13:24

Last fixes

61863aa

Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>

SdgJlbl force-pushed the feat/unify-user-dependencies branch from e110bdd to 61863aa Compare June 9, 2023 13:32

SdgJlbl merged commit 157cf59 into main Jun 9, 2023

SdgJlbl deleted the feat/unify-user-dependencies branch June 9, 2023 13:48

ThibaultFy added a commit that referenced this pull request Jun 14, 2023

Revert "feat!: unify user local dependencies (#123)"

096f2b0

This reverts commit 157cf59.

ThibaultFy added a commit that referenced this pull request Jun 14, 2023

Revert "feat!: unify user local dependencies (#123)"

73d54cb

This reverts commit 157cf59. Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>

ThibaultFy mentioned this pull request Aug 1, 2023

feat: unify dependencies #155

Merged

4 tasks

linear bot mentioned this pull request Sep 1, 2023

chore: wheel name from pip wheel output #160

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: unify user local dependencies #123

feat!: unify user local dependencies #123

SdgJlbl commented May 19, 2023 •

edited

Loading

linear bot commented Jun 5, 2023

ThibaultFy left a comment

ThibaultFy commented Jun 8, 2023

Owlfred commented Jun 8, 2023 •

edited

Loading

ThibaultFy left a comment

SdgJlbl commented Jun 8, 2023

Owlfred commented Jun 8, 2023 •

edited

Loading

SdgJlbl commented Jun 8, 2023

Owlfred commented Jun 8, 2023 •

edited

Loading

ThibaultFy commented Jun 9, 2023

Owlfred commented Jun 9, 2023 •

edited

Loading

SdgJlbl commented Jun 9, 2023

ThibaultFy commented Jun 9, 2023 •

edited

Loading

ThibaultFy left a comment

ThibaultFy Jun 9, 2023

SdgJlbl Jun 9, 2023

feat!: unify user local dependencies #123

feat!: unify user local dependencies #123

Conversation

SdgJlbl commented May 19, 2023 • edited Loading

Summary

Notes

Still TODO

Please check if the PR fulfills these requirements

linear bot commented Jun 5, 2023

ThibaultFy left a comment

Choose a reason for hiding this comment

ThibaultFy commented Jun 8, 2023

Owlfred commented Jun 8, 2023 • edited Loading

ThibaultFy left a comment

Choose a reason for hiding this comment

SdgJlbl commented Jun 8, 2023

Owlfred commented Jun 8, 2023 • edited Loading

SdgJlbl commented Jun 8, 2023

Owlfred commented Jun 8, 2023 • edited Loading

ThibaultFy commented Jun 9, 2023

Owlfred commented Jun 9, 2023 • edited Loading

SdgJlbl commented Jun 9, 2023

ThibaultFy commented Jun 9, 2023 • edited Loading

ThibaultFy left a comment

Choose a reason for hiding this comment

ThibaultFy Jun 9, 2023

Choose a reason for hiding this comment

SdgJlbl Jun 9, 2023

Choose a reason for hiding this comment

SdgJlbl commented May 19, 2023 •

edited

Loading

Owlfred commented Jun 8, 2023 •

edited

Loading

Owlfred commented Jun 8, 2023 •

edited

Loading

Owlfred commented Jun 8, 2023 •

edited

Loading

Owlfred commented Jun 9, 2023 •

edited

Loading

ThibaultFy commented Jun 9, 2023 •

edited

Loading