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

Enable parallel reading if requested, even if there are few documents #12796

Merged
merged 4 commits into from
Sep 18, 2024

Conversation

mgeier
Copy link
Contributor

@mgeier mgeier commented Aug 17, 2024

I would expect that if parallel reading is requested (and possible), that Sphinx actually does parallel reading.

However, it turns out that it doesn't do it if there are fewer than 6 documents.

The limit has been added 10 years ago by @birkenfeld in 1f23a5c, but I didn't find any motivation for it.

I found this quite confusing when trying to debug this issue: spatialaudio/nbsphinx#801

In many cases this won't be noticed because reading is fast, but in my case (executing Jupyter notebooks with nbsphinx in Sphinx's "reading" phase) it often is not fast.

Feature or Bugfix

  • Bugfix

@AA-Turner
Copy link
Member

Please could you also add a test of some description and a CHANGES entry?

A

chrisjsewell added a commit to useblocks/sphinx-needs that referenced this pull request Aug 21, 2024
Change duplicate need feedback from raising exceptions to emitting Sphinx warnings, e.g.

```
path/to/page.rst:4: WARNING: A need with ID STORY_PAGE_1 already exists, title: 'duplicate'. [needs.duplicate_id]
```

and ensure warning is also emitted during parallel builds

Note, although there was already a parallel build test in the test suite, 
actually it was not running parallel because there were not enough documents in the test project
(see sphinx-doc/sphinx#12796), so one more document is added.
mgeier and others added 2 commits August 24, 2024 16:55
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
@mgeier
Copy link
Contributor Author

mgeier commented Aug 24, 2024

Please could you also add a test of some description and a CHANGES entry?

I've added CHANGES in 2724858, but I don't know how to add tests for that.

Can you point me to existing tests or (even better!) add those tests yourself?

@picnixz
Copy link
Member

picnixz commented Aug 25, 2024

but I don't know how to add tests for that.

We are currently testing it in:

@pytest.mark.sphinx(
    'html',
    testroot='root',
    parallel=2,
)
def test_html_parallel(app):
    app.build()

The test is not really helpful though. In order to test, you can check whether _read_parallel or _read_serial was called using mocked objects (sorry, my previous comment was about write_parallel).

@mgeier
Copy link
Contributor Author

mgeier commented Aug 27, 2024

Thanks @picnixz for the hint!

However, this exceeds my Sphinx abilities and the amount of time I'm willing to invest.

@picnixz
Copy link
Member

picnixz commented Aug 28, 2024

I can try to find some time for that!

@jayaddison
Copy link
Contributor

I'm supportive of this suggestion, because I think it would help to discover parallelism-related nondeterminism issues.

To elaborate on that: we often request minimal repro case examples when attempting to debug issues, because it helps to narrow the scope of the relevant code. However: if the code behaves differently depending on input-related thresholds, then it becomes more difficult to determine what a minimal repro case is (both for the bugreporter and code reviewers / maintainers).

Short-term a change such as this could appear to make Sphinx less reliable - but I think it would be a forcing function to help us improve build reliability (a property that should not depend on the size of the documentation set).

@AA-Turner AA-Turner merged commit 7828761 into sphinx-doc:master Sep 18, 2024
21 of 22 checks passed
@mgeier mgeier deleted the parallel-read-with-few-documents branch September 18, 2024 18:41
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.

4 participants