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

👌 IMPROVE: declare parallel read safe #225

Merged
merged 3 commits into from
Oct 3, 2020

Conversation

rscohn2
Copy link
Contributor

@rscohn2 rscohn2 commented Sep 29, 2020

I am getting this warning:

WARNING: the sphinx_book_theme extension does not declare if it is safe for parallel reading, assuming it isn't - please ask the extension author to check and make it explicit

The PR declares the extension is parallel read safe, eliminating the warning and allowing parallel read.

I am not sure if the extension is parallel read safe, but it seems likely.

@welcome
Copy link

welcome bot commented Sep 29, 2020

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out EBP's Code of Conduct and our Contributing Guide, as this will greatly help the review process.

Welcome to the EBP community! 🎉

@rscohn2 rscohn2 changed the title declare parallel read safe [FIX] declare parallel read safe Sep 29, 2020
@choldgraf
Copy link
Member

ah yes - I suspect that it is as well. @chrisjsewell I feel like you have more experience w/ parallelism in Sphinx, any reason to think we can't set this to True?

@chrisjsewell
Copy link
Member

no it shouldn't be an issue, although of course you can always look to add a test case for parallel builds

@rscohn2
Copy link
Contributor Author

rscohn2 commented Sep 30, 2020

I added a test. It is very simple, but did find a problem in another extension. I tried, but was unable to add add it to pytest. You have to pass parallel=2 to the constructor of Sphinx, but I don't know how to pass it through.
As an alternative, I added it directly to the github ci config.

I have also tested on a 1500 page doc and it works fine.

The force push was to cleanup the commits addressing ci issues.

@chrisjsewell
Copy link
Member

but I don't know how to pass it through.

Yeh unfortunately it is not available via the test app:
https://github.com/sphinx-doc/sphinx/blob/d8c006f1c0e612d0dc595ae463b8e4c3ebee5ca4/sphinx/testing/util.py#L101

sphinx_book_theme/__init__.py Outdated Show resolved Hide resolved
@choldgraf
Copy link
Member

alright I think that we're all 👍 on the changes and tests are all happy, so I'm gonna merge this in. Thanks very much @rscohn2 !! much appreciated :-)

@choldgraf choldgraf changed the title [FIX] declare parallel read safe 👌 IMPROVE: declare parallel read safe Oct 3, 2020
@choldgraf choldgraf merged commit 7239788 into executablebooks:master Oct 3, 2020
@welcome
Copy link

welcome bot commented Oct 3, 2020

Congrats on your first merged pull request in this project! 🎉
congrats

Thank you for contributing, we are very proud of you! ❤️

graingert added a commit to graingert/dask-sphinx-theme that referenced this pull request Mar 22, 2023
Fixes dask#78

the upstream theme is also parallel safe
this theme doesn't add any event handlers so must also be safe

executablebooks/sphinx-book-theme#225
graingert added a commit to graingert/dask-sphinx-theme that referenced this pull request Mar 22, 2023
Fixes dask#78

the upstream theme is also parallel safe
this theme doesn't add any event handlers so must also be safe

executablebooks/sphinx-book-theme#225
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.

3 participants