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

chore: support Python 3.12 #226

Merged
merged 5 commits into from
Jul 24, 2024
Merged

chore: support Python 3.12 #226

merged 5 commits into from
Jul 24, 2024

Conversation

SdgJlbl
Copy link
Contributor

@SdgJlbl SdgJlbl commented Jun 28, 2024

No description provided.

@SdgJlbl SdgJlbl requested a review from a team as a code owner June 28, 2024 13:47
@SdgJlbl SdgJlbl force-pushed the chore/python3.12 branch 5 times, most recently from 748c669 to ab17af2 Compare July 23, 2024 14:12
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@SdgJlbl SdgJlbl force-pushed the chore/python3.12 branch 3 times, most recently from 0921c2d to 066a66e Compare July 23, 2024 15:24
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@@ -109,7 +109,7 @@ jobs:
steps:
- uses: actions/setup-python@v5
with:
python-version: 3.11
python-version: 3.12
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we rather rely on a python matrix there too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean, running the benchmark with all versions of Python?
I guess it wasn't done for cost effectiveness reasons, but it can be changed, indeed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's what I meant! (cf: comment on benchmark requirements)

Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@SdgJlbl
Copy link
Contributor Author

SdgJlbl commented Jul 24, 2024

/e2e --tests=substrafl,sdk,mnist,camelyon

@Owlfred
Copy link

Owlfred commented Jul 24, 2024

End to end tests: ✔️ SUCCESS

Awesome sauce!

benchmark/camelyon/requirements.txt Show resolved Hide resolved
Comment on lines +25 to +26
"substra~=0.54.0a1",
"substratools~=0.22.0a2",
Copy link
Contributor

Choose a reason for hiding this comment

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

To recap:

  • substratools in its alpha version is needed because we pip install it in the docker template
  • substra in its alpha version is needed because?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can try to make it work without the alphas, if we think it's worth it to invest the extra time 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that we do need the substratools release for the python setup in docker. I would like to understand why do we need the substra pre-release. If you don't know it's ok, feel free to resolve the comment.

@@ -109,7 +109,7 @@ jobs:
steps:
- uses: actions/setup-python@v5
with:
python-version: 3.11
python-version: 3.12
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's what I meant! (cf: comment on benchmark requirements)

Signed-off-by: SdgJlbl <sarah.diot-girard@owkin.com>
@SdgJlbl SdgJlbl merged commit 5c3d3e4 into main Jul 24, 2024
10 checks passed
@SdgJlbl SdgJlbl deleted the chore/python3.12 branch July 24, 2024 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants