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

[WIP] Decouple examples gallery from the main repository #251

Closed
wants to merge 9 commits into from

Conversation

aperezhortal
Copy link
Member

@aperezhortal aperezhortal commented Jan 4, 2022

Problem

Currently, the examples gallery is a part of the pysteps project and the examples are executed during the documentation build in Read The Docs (RTD). Running these examples is a time-consuming operation that puts us close to the RTD's build limits.

Proposed solution

The discussions in #242 converge on decoupling the example gallery from the main repo, keeping the examples in a separate repository that uses Github Actions to execute the examples and build the examples gallery. The rendered gallery is pulled from the separate repo when the pysteps documentation is built in RTD.

Changes introduced in this PR

  • Remove the examples files (examples/*.py files) from the pysteps repo. The examples were moved to the pysteps_tutorials repository. The pysteps_tutorials repository contains only the Sphinx documentation for the example gallery. Here, the GitHub actions render the example gallery (using sphinx-gallery) and save the rendered gallery (rst and png files) in separate branches. IMPORTANT, the examples have the form of python scripts for now. Although this can be updated to jupyter notebooks if needed.
  • Update the documentation build process to pull the rendered example gallery from the pysteps_tutorials repository.
  • See the generated documentation in https://pysteps.readthedocs.io/en/external_gallery/

Discussion

Should we use Jupyter notebooks or python scripts for the example gallery?

I personally prefer python scripts. Although jupyter notebooks are good for exploratory analysis and can be nicely visualized in Github, keeping track of the changes is very difficult since the diffs are not very human-readable. On the other hand, keeping track of the differences in python scripts is straightforward. Another advantage of using python scripts is that we can benefit from the static code analysis of the IDEs.

Also, sphinx-gallery automatically converts the python scripts to jupyter notebooks and makes them available in the documentation. There is an option (experimental) to add the open in binder button that we may explore.

I check how the example galleries are handled in other projects and some use jupyter notebooks while others use scripts:

  • wradlib: Jupyter notebooks in a separate repository.
  • Astropy: Jupyter notebooks in a separate repository.
  • fastai: Jupyter notebooks in the main repository.
  • Scikit-learn: Python scripts in the project repository.
  • Pytorch: Python scripts in the project repository.
  • Tensorflow: Python scripts in the project repository.
  • Matplotlib: Python scripts in the project repository.

It seems that it boils down to a matter of personal preference. Both approaches are good, and if we choose to use notebooks, updating the current PR is straightforward.

What are your thoughts?

@codecov
Copy link

codecov bot commented Jan 4, 2022

Codecov Report

Merging #251 (baedcdd) into master (bd94785) will not change coverage.
The diff coverage is n/a.

❗ Current head baedcdd differs from pull request most recent head c4e4aef. Consider uploading reports for the commit c4e4aef to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #251   +/-   ##
=======================================
  Coverage   80.78%   80.78%           
=======================================
  Files         140      140           
  Lines       10625    10625           
=======================================
  Hits         8583     8583           
  Misses       2042     2042           
Flag Coverage Δ
unit_tests 80.78% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd94785...c4e4aef. Read the comment docs.

@aperezhortal aperezhortal marked this pull request as ready for review January 6, 2022 12:27
@dnerini dnerini linked an issue Jan 8, 2022 that may be closed by this pull request
Copy link
Member

@dnerini dnerini left a comment

Choose a reason for hiding this comment

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

Fantastic job @aperezhortal!

I only have few understanding questions and there are a couple of super minor changes that we need to include (basically updating the references to the gallery page).

doc/source/user_guide/index.rst Show resolved Hide resolved
# Trigger the notebooks' execution in the external repository.
#
# If the "main" branch triggered this event, then the nb execution is triggered in the
# "main" branch, otherwise in the dev branch.
Copy link
Member

Choose a reason for hiding this comment

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

I really like this idea! So, did I understand correctly that all branches but master will automatically render into "dev"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. However, right now only the main branch is pull in the examples repo. I will add something to pull the same branch that sent the triggering commit to the external repo. In that way, we can test if the examples execute correctly with development branches.

@dnerini
Copy link
Member

dnerini commented Jan 8, 2022

I also noticed that pysteps_tutorials uses dev as default branch. Is this intentional?

@aperezhortal
Copy link
Member Author

I also noticed that pysteps_tutorials uses dev as default branch. Is this intentional?

Just a consequence of the branch that I was using 😄
I pushed the dev commits to the main branch, and set the latter as default.

@dnerini dnerini added this to the v1.6 milestone Jan 14, 2022
@dnerini
Copy link
Member

dnerini commented Jan 16, 2022

I have another annoying comment (sorry): it should be "Example gallery" all over instead of "Examples gallery", shouldn't it?

@aperezhortal
Copy link
Member Author

aperezhortal commented Jan 16, 2022

... it should be "Example gallery" all over instead of "Examples gallery"

Yes, you are right. I will fix it shortly.

I will also change the PR to Work In progress since I'm experimenting with a slightly different workflow where the rendering is triggered by empty commit messages to a dedicated branch. This allows finer control on which pysteps branch to pull, which branch in the gallery to render, and what RTD branch to trigger. 🤓

@aperezhortal aperezhortal changed the title Decouple examples gallery from the main repository [WIP] Decouple examples gallery from the main repository Jan 16, 2022
@aperezhortal
Copy link
Member Author

PR #262 implements a different workflow that I think will be easier to maintain since the example gallery is not decoupled.

Do we keep this PR open? Or do we close it in favor of #262?

@dnerini
Copy link
Member

dnerini commented Mar 16, 2022

Yes, I agree, let's close this PR in favor of #262.

@dnerini dnerini closed this Mar 16, 2022
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.

Decouple gallery from main documentation
2 participants