Skip to content

[BUG] redirect-from sphinx extension is not parallel safe #24058

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.

Sign up for GitHub

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

Jump to bottom
Closed
StefRe opened this issue Sep 30, 2022 · 8 comments · Fixed by #24054 or #24795
Closed

[BUG] redirect-from sphinx extension is not parallel safe #24058

StefRe opened this issue Sep 30, 2022 · 8 comments · Fixed by #24054 or #24795
Labels
Maintenance status: confirmed bug

Comments

@StefRe
Copy link
Contributor

StefRe commented Sep 30, 2022 •
edited
Loading

Summary

CI builds the docs in parallel since #20718.

This doesn't work correctly with redirects: in #24054 it raises

Traceback (most recent call last):
  File "/home/circleci/.local/lib/python3.8/site-packages/sphinx/cmd/build.py", line 279, in build_main
    app.build(args.force_all, filenames)
  File "/home/circleci/.local/lib/python3.8/site-packages/sphinx/application.py", line 350, in build
    self.builder.build_update()
  File "/home/circleci/.local/lib/python3.8/site-packages/sphinx/builders/__init__.py", line 302, in build_update
    self.build(to_build,
  File "/home/circleci/.local/lib/python3.8/site-packages/sphinx/builders/__init__.py", line 318, in build
    updated_docnames = set(self.read())
  File "/home/circleci/.local/lib/python3.8/site-packages/sphinx/builders/__init__.py", line 423, in read
    self._read_parallel(docnames, nproc=self.app.parallel)
  File "/home/circleci/.local/lib/python3.8/site-packages/sphinx/builders/__init__.py", line 479, in _read_parallel
    tasks.join()
  File "/home/circleci/.local/lib/python3.8/site-packages/sphinx/util/parallel.py", line 105, in join
    if not self._join_one():
  File "/home/circleci/.local/lib/python3.8/site-packages/sphinx/util/parallel.py", line 126, in _join_one
    raise SphinxParallelError(*result)
sphinx.errors.SphinxParallelError: ValueError: gallery/pyplots/dollar_ticks is already noted as redirecting to gallery/ticks/dollar_ticks

Sphinx parallel build error:
ValueError: gallery/pyplots/dollar_ticks is already noted as redirecting to gallery/ticks/dollar_ticks

(see also #21876)

Proposed fix

Revert afef2e6.
@StefRe StefRe mentioned this issue Sep 30, 2022
Merged
1 task
@jklymak
Copy link
Member

jklymak commented Sep 30, 2022

We should probably figure out why we get the error

Also note that since we introduced this mechanism, I think there is a proper package that does this. Whether they handle parallel better or not I'm not sure.

Finally, we don't host on GitPages any longer and could do this with .htaccess now if we chose (I think).

@timhoffm
Copy link
Member

It may be that our redirect directive is not thread-safe. Maybe we have to revert #19686 instead. - Either way, we definitively want parallelism for speed, and need to find a different solution.

@StefRe
Copy link
Contributor Author

StefRe commented Sep 30, 2022 •
edited
Loading

Please accept my apology. I included the same redirect in two different files. This error didn't show up for non-parallel builds: the second redirect just overwrites the first one. I fixed this mistake and the docs build in parallel without errors.

@StefRe StefRe closed this as completed Sep 30, 2022
@timhoffm
Copy link
Member

I'll reopen because the error hit again in a recent CI run.

I rename the issue to state the problem. Simply not building in parallel cannot be a solution because of the performance impact. The error seems rare enough so that the best temporary solution is continuing as is and restarting Circle CI when the error hits.

Mid-term, we should either make redirect-from parallel-safe or find a different solution. We cannot afford a sphinx extension that prevents doc builds in parallel.

@timhoffm timhoffm reopened this Dec 19, 2022
@timhoffm timhoffm changed the title [MNT]: Docs don't build in parallel redirect-from sphinx extension is not parallel safe Dec 19, 2022
@timhoffm timhoffm changed the title redirect-from sphinx extension is not parallel safe [BUG] redirect-from sphinx extension is not parallel safe Dec 19, 2022
@jklymak
Copy link
Member

jklymak commented Dec 19, 2022

Do we understand why this directive would not be parallel safe?

@oscargus
Copy link
Member

oscargus commented Dec 19, 2022 •
edited
Loading

What was the error in that recent run? I think that the original error is not related to parallel execution, but just re-thrown as a SphinxParallelError.

@timhoffm
Copy link
Member

timhoffm commented Dec 19, 2022 •
edited
Loading

In the recent run, the error was exactly the same as above.

The error is thrown from here

matplotlib/doc/sphinxext/redirect_from.py

Lines 92 to 103 in 2e64a9e

def run(self):
redirected_doc, = self.arguments
env = self.app.env
domain = env.get_domain('redirect_from')
current_doc = env.path2doc(self.state.document.current_source)
redirected_reldoc, _ = env.relfn2path(redirected_doc, current_doc)
if redirected_reldoc in domain.redirects:
raise ValueError(
f"{redirected_reldoc} is already noted as redirecting to "
f"{domain.redirects[redirected_reldoc]}")
domain.redirects[redirected_reldoc] = current_doc
return []

which should only be run once per .. redirect-from:: directive.

Maybe it has to do with using the domain for data storage and not handling that correctly on our side? At least https://www.sphinx-doc.org/en/master/development/tutorials/todo.html attaches its data todo_all_todos to the environment, not a domain.
Edit: No, sphinx-doc/sphinx#9003 (comment) suggests that attaching data to a domain is actually the correct way.

@timhoffm
Copy link
Member

Btw. it just hit again in a run of #24769:

https://app.circleci.com/pipelines/github/matplotlib/matplotlib/21068/workflows/9b157c38-fd55-4ba7-8cef-a002aa760b38/jobs/73024

@timhoffm timhoffm mentioned this issue Dec 20, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Maintenance status: confirmed bug
Projects
None yet
Milestone
No milestone
4 participants
@jklymak @timhoffm @oscargus @StefRe