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

chore: convert examples to notebook #368

Merged
merged 3 commits into from
Sep 29, 2023
Merged

chore: convert examples to notebook #368

merged 3 commits into from
Sep 29, 2023

Conversation

ThibaultFy
Copy link
Member

@ThibaultFy ThibaultFy commented Sep 20, 2023 •
edited
Loading

Description

For those who will have the courage to review this PRs... Thank you 🙏

Changes:

  • Convert examples to notebook
  • Move examples to docs/source/examples to be visible when building the documentation
  • Rm sphinx gallery dependency, in favour of nbsphinx. Nbsphinx needs accepts custom CSS files (in this PR). It also needs pandoc to be installed. The only way to install pandoc in RTD is with conda. We changed the behavior of RTD build in favour of a conda environment.
  • Update examples requirements files
  • Update example to pass from RST notation to markdown
  • Binder is back 💪

closes FL-1185

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@linear
Copy link

linear bot commented Sep 22, 2023

FL-1185 Convert examples to notebooks in documentation

Context

The examples are currently written in python file, in RST, to be build by sphinx in the expected shape.

One of the main reason why we don't use notebooks is because it is impossible to review directly on Github. But ReviewNB allows to review efficiently that kind of file, and is free to use for OS project. The integration with substra-documentation is already up and running.

The advantage of using nb would be:

  • remove the dependency with RST and allow to pass on markdown
  • remove the binder dependency and have the opportunity to use google colab or github spaces for users to run the example.

Specification

Convert all examples in notebooks.

Use review NB to review the PRs.

Adapt substra-ci to run the notebooks instead of the python code.

Acceptance criteria

@ThibaultFy ThibaultFy force-pushed the examples-to-notebook branch 16 times, most recently from 1062530 to e0f57d5 Compare September 25, 2023 12:04
@ThibaultFy
Copy link
Member Author

/e2e --tests doc

@Owlfred
Copy link

Owlfred commented Sep 25, 2023 •
edited
Loading

End to end tests: ✔️ SUCCESS

@ThibaultFy ThibaultFy force-pushed the examples-to-notebook branch 2 times, most recently from f8a4b6f to 2914ef3 Compare September 25, 2023 12:58
@Substra Substra deleted a comment from Owlfred Sep 25, 2023
@Substra Substra deleted a comment from Owlfred Sep 25, 2023
@Substra Substra deleted a comment from Owlfred Sep 25, 2023
@Substra Substra deleted a comment from Owlfred Sep 25, 2023
@Substra Substra deleted a comment from oleobal Sep 25, 2023
@ThibaultFy ThibaultFy marked this pull request as ready for review September 25, 2023 14:41
SdgJlbl
SdgJlbl approved these changes Sep 25, 2023
View reviewed changes
Copy link
Contributor

@SdgJlbl SdgJlbl left a comment

Choose a reason for hiding this comment

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

Impressive PR, nicely done 😮
Glad to be rid of sphinx-gallery, and good job on bringing Binder back from the dead.
Reviewing the notebooks separately since I'm not sure how NBreview will interact with comments here.

.binder/runtime.txt
@@ -1 +1 @@
python-3.8
python-3.10
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

docs/environment.yml Outdated
- defaults
- conda-forge
dependencies:
- python=3.8
Copy link
Contributor

Choose a reason for hiding this comment

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

🧐
do we want python 3.10 here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch, this is a leftover when I tried different config to make it work (the key was to select mambaforge-22.9 in the rtd.yaml

SdgJlbl
SdgJlbl reviewed Sep 25, 2023
View reviewed changes
docs/source/examples/substra_core/diabetes_example/run_diabetes.ipynb
@@ -0,0 +1,725 @@
{
Copy link
Contributor

@SdgJlbl SdgJlbl Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Nor this one, technically


Reply via ReviewNB

docs/source/examples/substra_core/titanic_example/run_titanic.ipynb
@@ -0,0 +1,568 @@
{
Copy link
Contributor

@SdgJlbl SdgJlbl Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think we are not using matplolib on this specific example


Reply via ReviewNB

docs/source/examples/substrafl/get_started/run_mnist_torch.ipynb
@@ -0,0 +1,797 @@
{
Copy link
Contributor

@SdgJlbl SdgJlbl Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Line #3.    dependencies = Dependency(pypi_dependencies=["numpy==1.23.1", "torch==1.11.0", "scikit-learn==1.1.1"])

1.11? maybe we want to update to 1.13.1 (at least until we bump it to 2+)


Reply via ReviewNB

docs/source/examples/substrafl/go_further/run_diabetes_substrafl.ipynb
@@ -0,0 +1,698 @@
{
Copy link
Contributor

@SdgJlbl SdgJlbl Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Compute Plan Builder and Decorator are not displaying correctly, for some reasons


Reply via ReviewNB

docs/source/examples/substrafl/go_further/run_iris_sklearn.ipynb
@@ -0,0 +1,679 @@
{
Copy link
Contributor

@SdgJlbl SdgJlbl Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, the missing coma is probably on all notebooks, given that this paragraph has been copy-pasted everywhere


Reply via ReviewNB

docs/source/examples/substrafl/go_further/run_mnist_cyclic.ipynb
@@ -0,0 +1,1113 @@
{
Copy link
Contributor

@SdgJlbl SdgJlbl Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If not, follow the instructions described here. (missing coma)


Reply via ReviewNB

docs/source/examples/substrafl/go_further/run_mnist_cyclic.ipynb
@@ -0,0 +1,1113 @@
{
Copy link
Contributor

@SdgJlbl SdgJlbl Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Link generation problem here too, for TorchAlgorithm


Reply via ReviewNB

@ThibaultFy
chore: move examples and create md notebooks
8d1ded0 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
chore: configure examples
b85caae 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy
chore: sarah review on notebooks
3f541d2 
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy ThibaultFy merged commit 67116b6 into main Sep 29, 2023
3 checks passed
@ThibaultFy ThibaultFy deleted the examples-to-notebook branch September 29, 2023 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RomainGoussault RomainGoussault Awaiting requested review from RomainGoussault RomainGoussault is a code owner

@SdgJlbl SdgJlbl Awaiting requested review from SdgJlbl

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