Skip to content

FIX Copy JupyterLite contents early so it runs before jupyterlite_sphinx build-finished#1213

Merged
larsoner merged 7 commits intosphinx-gallery:masterfrom
lesteve:fix-jupyterlite-sphinx-build-finished-order
Oct 24, 2023
Merged

FIX Copy JupyterLite contents early so it runs before jupyterlite_sphinx build-finished#1213
larsoner merged 7 commits intosphinx-gallery:masterfrom
lesteve:fix-jupyterlite-sphinx-build-finished-order

Conversation

@lesteve
Copy link
Copy Markdown
Member

@lesteve lesteve commented Oct 23, 2023

Fix #1211.

I am not sure how to add a tests for this.

You want the notebooks copied to jupyterlite_contents copied before the jupyterlite_sphinx runs jupyterlite build --contents jupyterlite_contents.

I could also copy the contents in builder-inited after generate_gallery_rst not sure which ones is less brittle.

@lesteve
Copy link
Copy Markdown
Member Author

lesteve commented Oct 23, 2023

The CI errors are not related to this PR, probably an issue with libmamba ...

ImportError: /usr/share/miniconda/lib/python3.11/site-packages/libmambapy/../../../libmamba.so.2: undefined symbol: solver_ruleinfo2str, version SOLV_1.0

Looking around, I did not find too much about these errors, conda-incubator/conda-store#632 mentions that is started happening a few days ago. It was worked around in conda-incubator/conda-store#634 by not mixing not mixing defaults and conda-forge.

@larsoner larsoner added the bug label Oct 23, 2023
@larsoner larsoner mentioned this pull request Oct 23, 2023
@larsoner
Copy link
Copy Markdown
Contributor

CI failures should be fixed by #1214

Copy link
Copy Markdown
Contributor

@lucyleeow lucyleeow left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks

app.connect("builder-inited", generate_gallery_rst)
app.connect("build-finished", copy_binder_files)
app.connect("build-finished", create_jupyterlite_contents)
# Setting a small priority so that create_jupyterlite_contents runs before
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Setting a small priority so that create_jupyterlite_contents runs before
# Setting a low priority so that create_jupyterlite_contents runs before

Just to avoid confusion.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So I used small here on purpose rather than low. Callbacks are run in order of ascending priority so small priority means high priority (priority=100 means higher priority than priority=500 which is the default).

Maybe "setting priority to a small number" is less confusing. There is a similar in a comment a few lines above so it could be improved as well ...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah I got myself confused, even after reading the docs.

"setting priority to a small number" sounds good!

Copy link
Copy Markdown
Member Author

@lesteve lesteve Oct 24, 2023

Choose a reason for hiding this comment

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

OK I tweaked the comments hopefully helping to avoid the confusion.

For reference, the connect doc says:

priority: The priority of the callback. The callbacks will be invoked in order of priority (ascending).

@lesteve
Copy link
Copy Markdown
Member Author

lesteve commented Oct 24, 2023

Hmmm actually thinking about it I think we also want copying the notebooks to be after generating the notebooks, I think the priority needs a bit more tweaking ...

@lesteve
Copy link
Copy Markdown
Member Author

lesteve commented Oct 24, 2023

Hmmm actually thinking about it I think we also want copying the notebooks to be after generating the notebooks, I think the priority needs a bit more tweaking ...

I think everything is good to go actually after thinking more about it. The notebooks are generated by generate_gallery_rst at builder-inited and notebooks are copied at builder-finished time by create_jupyterlite_contents.

@larsoner larsoner enabled auto-merge (squash) October 24, 2023 14:51
@larsoner larsoner merged commit 6230eb0 into sphinx-gallery:master Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Order dependence with Jupyterlite

3 participants