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

Run notebooks on RTD with sphinx-gallery and add WorkGraph tutorials #474

Open
wants to merge 39 commits into
base: main
Choose a base branch
from

Conversation

agoscinski
Copy link
Contributor

@agoscinski agoscinski commented Sep 11, 2024

Similar as in aiida-workgraph aiidateam/aiida-workgraph#234 I would like to integrate an execution of the notebooks as the notebooks become quickly outdated otherwise. This simplifies the update process of the notebooks as they will be automatically run with each CI, so errors can be immediately seen and then fixed. Since we already adapted the tutorials from the aiida-workgraph to the sphinx-scripts and I would like to move these tutorials out of aiida-workgraph to this repo as fast as possible I focused on moving these as a first example. Nevertheless I would like to adapt all tutorials to be executable in the future.

I tried myst_nb for the execution as it was already provided by the repo, but I ran into issues that ran explained below, so I used the same strategy as in aiida-workgraph by using sphinx-gallery.

Note that the sections Writing workflows and Writing workflows with WorkGraph (the new section including the WorkGraph) have overlaps that could be better resolved, but I did not want to spend the time in the first PR setting up the project structure, since I feel like this could become very time consuming.

Also Note that the CI link checker has been merged to RTD. Since for link checking one also needs to run the tutorials, I thought this is less maintenance as one just needs to update the RTD setup. This also does not affect the building time, because on the second build when creating the html files, the files from the first build can be Reused. Compare the times of https://readthedocs.org/projects/aiida-tutorials/builds/25605020/ of two builds (linkcheck plus html) and https://readthedocs.org/projects/aiida-tutorials/builds/25603891/ one build (only html)

@agoscinski
Copy link
Contributor Author

I tried using myst_nb but I get WARNING: No source code lexer found for notebook cell 1 [myst-nb.lexer] warnings which I cannot find any solution to online. It seems that I need to change markdown part in the cells, but I am really not sure. Also when an error is thrown it is written into a file and not printed out, which is really crucial for debugging runs on RTD. So I am switching to sphinx-gallery.

@agoscinski agoscinski changed the title WIP: run notebooks on RTD Run notebooks on RTD with sphinx-gallery and add WorkGraph tutorials Sep 12, 2024
# only pushes to main trigger
branches: [main]
pull_request:
# always triggered
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is unrelated to this PR. It stops that the CI is run two time for each PR. I will put this in a separate PR

@@ -11,29 +16,8 @@ jobs:

steps:
- uses: actions/checkout@v2
- name: Set up Python 3.12
- name: Set up Python 3.11
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To build aiida-core in CI, I needed to use Python 3.11. For pre-commit it is not needed but I think we should just stick with Python 3.11 if there are issues with Python 3.12.

- uses: pre-commit/[email protected]

build:
Copy link
Contributor Author

@agoscinski agoscinski Sep 12, 2024

Choose a reason for hiding this comment

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

This is now run in the RTD, check out the .readthedocs.yml

.readthedocs.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@@ -68,7 +69,7 @@
ipython_mplbackend = ""

copybutton_selector = "div:not(.no-copy)>div.highlight pre"
copybutton_prompt_text = ">>> |\.\.\. |\$ |In \[\d*\]: | {2,5}\.\.\.: | {5,8}: "
copybutton_prompt_text = ">>> |... |$ |In [d*]: | {2,5}...: | {5,8}: "
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The escapes raised some warnings

"https://www.big-map.eu/",
r".*concept/index.html",
r".*howto/index.html",
"http://127.0.0.1:8000/workgraph",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we ignore workgraph related files

docs/conf.py Outdated Show resolved Hide resolved
from pathlib import Path


def copy_html_files(app, exception):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

copy paste from aiida-workgraph to move html files from the sphinx gallery folder to the sphinx source, so we can display the widgets


================================
Running workflows with WorkGraph
================================
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sphinx-gallery requires a GALLERY_HEADER.rst but we do not use if anywhere therefore the :orphan: at the top to surppres warnings

@@ -0,0 +1,283 @@
"""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

copy paste from aiida-workgraph tutorials as all sphinx-gallery scripts

requirements.txt Outdated Show resolved Hide resolved
# to run notebooks
aiida-quantumespresso
aiida-pseudo
aiida-workgraph[widget]
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 think aiida-workgraph we do not want to pin, but I am not sure about the others

@agoscinski agoscinski marked this pull request as ready for review September 12, 2024 12:39
^^^^^^^^^^^^

A short module on how to write the basic type of workflows in AiiDA: work functions.
The module also revises the usage of calculation functions to add simple Python functions to the provenance.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This text needs some rework, maybe one of the reviewer has some ideas

^^^^^^^^^^^^

A step-by-step introduction to the basics of writing work chains in AiiDA.
After completing this module, you will be ready to start writing your own scientific workflows!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This text needs some rework, maybe one of the reviewer has some ideas

@superstar54
Copy link
Member

Hi @agoscinski , thanks for the work.

I tried myst_nb for the execution as it was already provided by the repo, but I ran into issues that ran explained below, so I used the same strategy as in aiida-workgraph by using sphinx-gallery.

What's the issue? Can we fix it? It's better to use the same method for all, to make it easy to maintain.

Refactor the notebooks

There are duplicate contents in the three notebooks:

  • EOS workgraph in both zero-to-hero and the eos notebooks.
  • atomization energy workgraph in both zero-to-hero and computational materials science notebooks.
    I would suggest replacing the zero-to-hero with workgraph basic.

Compare with WorkChain

Since the previous section is WorkChain, and it also shows examples of simple add and real-world EOS:

  • There are some calcfunctions already defined in the WorkChain section; use them, if possible, to ensure consistency.
  • Mention the WorkChain version, or try to compare them with WorkGraph, so that users know why WorkGraph makes it simple.

Use same style as the other section

The Workchain section is well-written, explaining steps step by step and providing extensive descriptions. It also has Note, Important and explanation. I suggest using the same style to make the website more uniform.
Also, the font size of the title/subtitle, etc., should be the same.

To summarize, I would suggest rewriting the WorkGraph section to fit into the aiida-tutorial better.

Comment on lines +519 to +527
# What's Next
# ===========
#
# +-----------------------------------------------------+-----------------------------------------------------------------------------+
# | `Concepts <../../concept/index.html>`_ | A brief introduction of WorkGraph’s main concepts. |
# +-----------------------------------------------------+-----------------------------------------------------------------------------+
# | `HowTo <../../howto/index.html>`_ | Advanced topics, e.g., flow control using `if`, `while`, and `context`. |
# +-----------------------------------------------------+-----------------------------------------------------------------------------+
#
Copy link
Member

Choose a reason for hiding this comment

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

This is not valid anymore; maybe remote it.
Similar to the WorkChain, we may add some exercises.

@agoscinski
Copy link
Contributor Author

agoscinski commented Sep 19, 2024

What's the issue? Can we fix it? It's better to use the same method for all, to make it easy to maintain.

Hey Xing I wrote it in the second post.

I tried using myst_nb but I get WARNING: No source code lexer found for notebook cell 1 [myst-nb.lexer] warnings which I cannot find any solution to online. It seems that I need to change markdown part in the cells, but I am really not sure. Also when an error is thrown it is written into a file and not printed out, which is really crucial for debugging runs on RTD. So I am switching to sphinx-gallery.

Note that myst_nb is also only used for one notebook here and not well integrated.


Thanks for the suggestions Xing, but this PR is not intended to refactor the existing examples as I mentioned.

Note that the sections Writing workflows and Writing workflows with WorkGraph (the new section including the WorkGraph) have overlaps that could be better resolved, but I did not want to spend the time in the first PR setting up the project structure, since I feel like this could become very time consuming.

We can put your comments in an issue.

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.

2 participants