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

Documentation set up with Sphinx #200

Merged
merged 28 commits into from
Feb 2, 2024
Merged

Conversation

melissawm
Copy link
Member

Supersedes #173

May close #80

I haven't really looked at the scrapers, because currently the docs execute even if the images are not in the gallery. Feel free to push to this PR if you know how to build the animation scraper. I can also try to come up with a solution later.

cc @GenevieveBuckley

docs/index.md Outdated
@@ -0,0 +1,135 @@
# napari-animation documentation
Copy link
Contributor

Choose a reason for hiding this comment

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

I really don't like that this docs/index.md file is an almost-identical copy of the README.md. This is a recipe for updates/changes being made in one place but not the other, and eventually ending up with users/developers having only partial information (depending on whether they mostly look at the built docs website, or mostly look at the github repo home page).

Can we change this, either to point sphinx directly to the main README.md in the repository, or somehow have the contents actually copied across during the build process?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I didn't know what to write here so just copied the README as a placeholder. I can do an include, but there are some sections that won't work, for example the admonitions (since github markdown and myst markdown don't agree on that syntax)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so we can choose between:

  1. Use the include directive in index.md, keep README.md and choose whether to have
    1A) Have bad admonition formatting on the website, or...
    1B) Have bad admonition formatting on the github page. Personally, I'd prioritize making the official website look good.
  2. Use the include directive in index.md, and convert the README to ReStructured text. Then the formatting could look good both on the website and on the github page.

Copy link
Member

Choose a reason for hiding this comment

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

1B) Have bad admonition formatting on the github page. Personally, I'd prioritize making the official website look good.

This would be my choice also.

docs/conf.py Outdated Show resolved Hide resolved
https://sphinx-gallery.github.io/stable/advanced.html#example-2-detecting-image-files-on-disk
"""
# Find all video files in the directory of this example.
video_file_extensions = [".mp4", ".mov"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I checked this works for .mp4 and .mov filetypes. We could add more, so long as someone checks they work well in the built docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Although for some reason, when I build the documentation with make docs there are warnings output about the .mov file format.

I'm not sure why, because the HTML website works as expected, regardless of whether we have .mov or .mp4 videos embedded. Regardless, I've changed the output files from our example scripts to use .mp4, just to avoid the warnings in the docs build.

rst = f""".. video:: {video_filepath}
:autoplay:
:loop:
:width: 600
Copy link
Contributor

@GenevieveBuckley GenevieveBuckley Jan 25, 2024

Choose a reason for hiding this comment

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

I don't like this part.
Currently it sets the width of the embedded video to 500 pixels. I'd prefer it was a percentage of the page width instead (i.e. more responsive to different screen sizes). I don't think you can do that easily with sphinxcontrib-video as it currently exists, so maybe we should replace this dependency with a template docstring of the HTML we need to embed in the RST document, so we can have more control over the output result.
Docs: https://github.com/sphinx-contrib/video

@GenevieveBuckley
Copy link
Contributor

GenevieveBuckley commented Jan 25, 2024

I don't see the ipython notebook example int he built html docs. I had expected to see it appear as a page in the examples gallery. @melissawm is this part working for you?

EDIT: Ah, I see now. Any jupyter notebook files are being deliberately skipped (see setup function in docs/conf.py)

def setup(app):
    """Set up docs build.
    * Ignores .ipynb files to prevent sphinx from complaining about multiple
      files found for document when generating the gallery
    * Rewrites github anchors to be comparable
    * Adds google calendar api key to meetings schedule page
    """
    app.registry.source_suffix.pop(".ipynb", None)

I thought we wanted the notebooks to be executed and included? #70

I see that there is a single line of text in the gallery.rst page linking to it, but that is easy to overlook. It also leaves us vulnerable to the code in that notebook going out of date, and we won't know it produces errors.

@GenevieveBuckley
Copy link
Contributor

GenevieveBuckley commented Jan 29, 2024

Summarizing the current status (Melissa, please add or correct anything I've got wrong here)

Items remaining for this PR:

  • Add Fix Github actions script to build and deploy docs (presumably will be based on this docs deploy script), so it does not rely on a secret github key
  • Set up Github secret key for docs deployment (apparently I don't have the right permissions for this - @jni who does?) Juan thinks we don't need this.
  • Resolve README duplication (discussion thread here). Done. I've chosen to prioritize the HTML rendering, so the github markdown rendering looks unformatted for those sections, although all the information is still there.
  • Are the linkcheck_ignore and rewrite_github_anchor functions staying or going (see discussion thread here). Done, they can be removed. It doesn't even look like the main napari/docs repo is actually running a link checker anyway, and we don't have one here yet either.

Nice to have, possibly in a later PR:

  • I can modify the VideoScraper class to close any open napari windows if an output video file is found, then run the regular static napari screenshot scraper for all other cases. Is that behaviour we'd want? (It's easy for me to do, I just want to know if that's what other people think should happen). Done.
  • Responsive (or percentage based) sizing of embedded videos. This currently isn't supported by the sphinx video plugin, so we'd have to either modify that or write our own HTML template string. (See here)
  • Executing .ipynb jupyter notebook files. I'm not sure what the reason for skipping these in the napari/docs is. There might be some tricky technical issue preventing it here too? See this line of code in conf.py.

@jni
Copy link
Member

jni commented Jan 31, 2024

Set up Github secret key for docs deployment (apparently I don't have the right permissions for this - @jni who does?)

I don't think you actually need this to deploy to the same-repo's gh-pages. See the deploy to github pages docs (though they are not as clear as I'd like about this):

Alternatively you can set the following in your workflow file to grant the action the permissions it needs.

permissions:
  contents: write

To answer your Q, the napari steering council can do that if needed. But I think it's not. 😅

@GenevieveBuckley
Copy link
Contributor

Ok, maybe something is funky about how the napari/docs repository handles this, that's where the secret key business came from. If someone would like to edit the new deploy_docs.yml file here to remove that requirement, that would be fantastic.

@jni
Copy link
Member

jni commented Jan 31, 2024

Yeah the issue with the napari/docs repo is that it needs to deploy to a different repo. Is that the case here or are we just deploying to the gh-pages branch?

@melissawm
Copy link
Member Author

@jni is right, this can be simplified. Unfortunately I won't have time this week to work on this PR but I can take a look next week if it's still pending. Thanks for the fixes @GenevieveBuckley !

@GenevieveBuckley
Copy link
Contributor

GenevieveBuckley commented Feb 1, 2024

I've made some progress towards getting the deployment working.

  • Pack multiple folders into the github pages artifact upload (we need all of docs/_build, docs/_static, and docs/gallery)
  • Fix absolute vs relative path issue with HTML video embeddings (now we use relative paths for better portability, which we need for the deployment)
  • Make sure github pages can find our index.html file. Because we bundle multiple directories into the artifact, I think github pages might not know where to find our main page _build/index.html.
  • Fix "Error: Artifact could not be deployed. Please ensure the content does not contain any hard links, symlinks and total size is less than 10GB." Turns out this error message was misleading, it's just that the artifact did not begin with ./ Switching to actions/upload-pages-artifact helped fix the issue.
    • The uncompressed build artifact is ~20MB (the compressed zip file is about half that). I don't think the total size of the artifact is the problem.
    • I tried to check the artifact for symlinks, using ls -lR /path/to/folder | grep '^l' as suggested by the top stackoverflow answer here. I didn't find anything, but I'm also not sure if this was the right way to check or not.
    • I'm not sure how to check if the artifact contains hard links

I've uploaded the most recent artifact and the CI logs here on google drive. You can open the html website locally in your browser and see that it all looks correct and the gallery videos play.

If anyone can help figure out why the artifact is causing problems, that would be wonderful.

Note: I'm using actions/upload-artifact instead of the suggested actions/upload-pages-artifact, mostly because I was finding it tricky to include multiple directories (we need our build, static, and gallery files). I don't think that could be causing the problem, but should mention it.

Finally, I've made a copy of the repo (not a fork, it won't deploy github pages on a fork) to play with things here: https://github.com/GenevieveBuckley/napari-animation-copy/ I can add people to this repo if you like.

@GenevieveBuckley
Copy link
Contributor

GenevieveBuckley commented Feb 2, 2024

I've got it working now - you can see a working test website here: https://genevievebuckley.github.io/napari-animation-copy/gallery.html EDIT: I've deleted the test website now the main website here is working https://napari.org/napari-animation/gallery.html

In the end it was simplest to add a command in the Makefile to copy the video files across into docs/_build/gallery/images, then point the actions/upload-pages-artifact to just the docs/_build folder (which now contains everything we need).

@GenevieveBuckley GenevieveBuckley merged commit 1aa014e into napari:main Feb 2, 2024
2 of 14 checks passed
@GenevieveBuckley
Copy link
Contributor

It works! 🎉 🚀
https://napari.org/napari-animation/gallery.html

@psobolewskiPhD
Copy link
Member

Awesome! 🎉

GenevieveBuckley added a commit that referenced this pull request Feb 7, 2024
Follow up to #200

Some updates for the README, related to the new docs website.
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.

None yet

4 participants