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

Use pangeo_sphinx_book_theme and mock imports #62

Merged
merged 9 commits into from
May 18, 2022

Conversation

maxrjones
Copy link
Member

The docs for the PyTorch data loaders aren't currently rendered at https://xbatcher.readthedocs.io/en/latest/api.html#dataloaders due to the following exception:

WARNING: autodoc: failed to import class 'torch.MapDataset' from module 'xbatcher.loaders'; the following exception was raised:
No module named 'torch'
WARNING: autodoc: failed to import class 'torch.IterableDataset' from module 'xbatcher.loaders'; the following exception was raised:
No module named 'torch'

This PR adds pytorch to doc/environment.yml to fix that problem. It also makes two minor changes -

  • add sphinx_rtd_theme to doc/environment.yml so that the same file can be used by contributors to set up an environment and build the docs locally.
  • ignore and clean the folder doc/generated/, which contains built html files for the xarray accessors.

@andersy005
Copy link
Member

add sphinx_rtd_theme to doc/environment.yml so that the same file can be used by contributors to set up an environment and build the docs locally.

it might be worth switching to the furo theme. it's very neat : https://pradyunsg.me/furo/quickstart/ :)

Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Documentation render at https://xbatcher--62.org.readthedocs.build/en/62/api.html#dataloaders looks good now.

To make the build a bit faster (considering that pytorch is a heavy dependency), I'd recommend refreshing the configuration in the readthedocs.yml file to use mamba instead of conda, see https://docs.readthedocs.io/en/stable/guides/conda.html#making-builds-faster-with-mamba. Specifically, need to modify these lines https://github.com/pangeo-data/xbatcher/blob/34ca3f9b6acaf23b09ef140b69bc7c79d4f38feb/readthedocs.yml#L1-L5

But that could be done in a follow-up Pull Request.

@rabernat
Copy link
Contributor

rabernat commented May 4, 2022

To make the build a bit faster

The best way to make it faster would be to not install pytorch at all and just use autodoc_mock_imports 😄 https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#confval-autodoc_mock_imports

@maxrjones
Copy link
Member Author

The best way to make it faster would be to not install pytorch at all and just use autodoc_mock_imports 😄

Nice, thanks! Looks good after 85d3db6

@@ -36,7 +36,6 @@ repos:
rev: v2.6.2
hooks:
- id: prettier
language_version: system
Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should either remove the requirement to have a system nodejs installation for pre-commit to work or add a contributing guide (xref #32) that includes a pre-commit explainer.

@codecov
Copy link

codecov bot commented May 5, 2022

Codecov Report

Merging #62 (8ffec20) into main (34ca3f9) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main       #62   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines          134       134           
  Branches        30        30           
=========================================
  Hits           134       134           

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 34ca3f9...8ffec20. Read the comment docs.

@maxrjones maxrjones changed the title Fix loader docs Update xbatcher docs setup May 6, 2022
@maxrjones maxrjones changed the title Update xbatcher docs setup Update docs setup May 6, 2022
@maxrjones
Copy link
Member Author

@andersy005's suggestion to use the furo theme can be previewed here - https://xbatcher--62.org.readthedocs.build/en/62/.

@maxrjones
Copy link
Member Author

@andersy005's suggestion to use the furo theme can be previewed here - https://xbatcher--62.org.readthedocs.build/en/62/.

While I'm also generally a fan of the furo theme, I reverted the last commit to restore the sphinx_rtd_theme configuration because the default settings for dark mode make it difficult to see the rendered notebook content (example below). While it would be possible to modify the configuration for improved readability, I think it's best to keep the PR as small as possible.

image

@rabernat
Copy link
Contributor

@weiji14
Copy link
Member

weiji14 commented May 10, 2022

@andersy005's suggestion to use the furo theme can be previewed here - https://xbatcher--62.org.readthedocs.build/en/62/.

While I'm also generally a fan of the furo theme, I reverted the last commit to restore the sphinx_rtd_theme configuration because the default settings for dark mode make it difficult to see the rendered notebook content (example below). While it would be possible to modify the configuration for improved readability, I think it's best to keep the PR as small as possible.

This should be fixed with the next xarray release (2022.05.0?). See pydata/xarray#6500 and pydata/xarray#6501. But agree with keeping this PR small and changing the theme in another PR.

@maxrjones
Copy link
Member Author

Thanks Wei Ji, it's good to know about that fix for the HTML repr.

Great, I'll modify the theme to pangeo-sphinx-book-theme for consistency with the other sites. I didn't notice an obvious default from the other repos in the pangeo github org, so it's helpful to have the pointer to that option.

@maxrjones
Copy link
Member Author

The readthedocs previous does not yet reflect the latest changes, likely due to approval being required for the workflows, but in my opinion the local build looks really nice with the pangeo-sphinx-book-theme. I would greatly appreciate a PR review whenever someone with write permissions has time, mostly to benefit from the updates while working on #42.

@jhamman
Copy link
Contributor

jhamman commented May 18, 2022

This looks so good @meghanrjones! Thanks for sorting out the theme and torch mock.

@jhamman jhamman merged commit f8c45db into xarray-contrib:main May 18, 2022
@maxrjones maxrjones deleted the fix-loader-docs branch October 7, 2022 15:38
@maxrjones maxrjones changed the title Update docs setup Use pangeo_sphinx_book_theme and mock imports Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants