Skip to content

Is it possible to control the "force" resp. "-E" option for Sphinx? (turns out it's actually the "-d" option) #4363

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

Closed
mgeier opened this issue Jul 13, 2018 · 19 comments
Assignees
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Milestone

Comments

@mgeier
Copy link
Contributor

mgeier commented Jul 13, 2018

My builds take quite a bit of time and memory because I'm using a custom Sphinx source parser that executes Jupyter notebooks (http://nbsphinx.readthedocs.io/).

I've seen that the -E option is used when calling Sphinx, which means that for every output format (HTML, JSON, PDF, ...) the source files are parsed (and in my case the Jupyter notebooks are executed) again and again.

Parsing the source files multiple times doesn't (and shouldn't!) change anything, it just burns some of my (and your) precious build time and memory.

Is it possible to disable this on a per-project basis?

I guess there is some reason to do this in the first place, but if not, it should probably be deactivated globally?

I've looked through the source code and found that this is controlled by the force argument to the builder class and just about everywhere the default is force=False.
But it seems that the actual build process is somehow started with force=True.

@stsewd
Copy link
Member

stsewd commented Jul 13, 2018

mmm, I always thought that option is for reuse with the same builder only (not with different builders).

@mgeier
Copy link
Contributor Author

mgeier commented Jul 13, 2018

No, Sphinx parses all source files and stores the "document" (and the "environment") in a representation that's independent of the builder.

Normally that doesn't make a big difference, since parsing the source files is quite fast, but in my case it's not.

But creating the HTML output and the LaTeX output is very quick if the "environment" is unchanged. In my case that could easily save several minutes build time.

@humitos
Copy link
Member

humitos commented Jul 24, 2018

I'd like to take a deeper look at this, but at first sight, I think it's at least, weird :). I already wanted to remove the force attribute some time ago but I left it because I wasn't sure how it was used.

Today, I found that the only place where force=True is when the build is triggered by a webhook:

https://github.com/rtfd/readthedocs.org/blob/8a34164f73f886afea8c2f923e7d42ac7b9ca108/readthedocs/core/views/hooks.py#L45

So, I suppose that if you trigger a build manually force will be False. It'd be a good experiment to do and see how much is the difference in building time. Could you provide your RTD's project URL here?

I've seen some projects having problems with memory limits in the past weeks and maybe this could be one of the reasons (#4403)

@humitos
Copy link
Member

humitos commented Jul 24, 2018

Even if we remove the -E globally/completely, all the "first time" builds will create the environment from scratch.

"First time" build will happen regularly since RTD cleans it cache after a couple of hours if the project didn't triggered a build. Also, we currently have 4 build servers, so if the task is executed in 1 and then in 2, those two will be "first time" builds.

That said, I'm not sure that removing this option will change the building time/memory used too much but just make RTD builds fail randomly instead.

@stsewd
Copy link
Member

stsewd commented Jul 24, 2018

@humitos since the source files are preserved in the first builder (html), the further builders will reuse that (pdf, json, singlehtml, etc), that would save some time in the same build process each time.

@humitos
Copy link
Member

humitos commented Jul 24, 2018

@stsewd that's true, but does the same apply for the nbsphinx? I mean, I understand that nbsphinx is a plugin that runs inside the HTML builder (which is the first one in our steps) so, for this particular case it won't make any difference in memory usage, right?

@mgeier
Copy link
Contributor Author

mgeier commented Jul 26, 2018

TL;DR: I think I found the solution: all builders should use the same -doption!

@humitos

Could you provide your RTD's project URL here?

Sure, I should have provided those in my original comment.

It's https://splines.readthedocs.io/, an example build is https://readthedocs.org/projects/splines/builds/7518379/.

I didn't notice this before, but it looks like the -E option is only used in the first call to Sphinx (using the readthedocs builder) but it isn't used in the second call (using the json builder).
However, even though -E is not used in the second call, the source files are parsed again anyway.

After having a closer look, I now think I know what's the problem: each Sphinx call on RTD seems to use a separate -d option.

That means each builder uses their own "environment" and therefore it can never be re-used between builders!

I think this is a big waste of resources, isn't it?

Here is another example: https://readthedocs.org/projects/nbsphinx/builds/7532399/
Again, the -E flag is only used in the first invocation of Sphinx, but the source files are re-parsed anyway in the following invocations (most likely because separate -d options are used).

That said, I'm not sure that removing this option will change the building time/memory used too much but just make RTD builds fail randomly instead.

If done correctly, this should cause a significant reduction in building time.

I think it makes sense to re-build the "environment" for each new version of the project, because problems could arise with certain changes of the configuration. I guess the current usage of -E makes sense for that.

But it should be safe to re-use the "environment" for all builders of one given version. While the current usage of -E can be kept as is, the usage of -d should be changed for that.

@stsewd

since the source files are preserved in the first builder (html), the further builders will reuse that (pdf, json, singlehtml, etc), that would save some time in the same build process each time.

They should re-use the "environment" of the first builder, but apparently they don't.
The -E flag seems to be used only for the first builder, which is OK.
But since each builder uses their own -d options the "environment" is never re-used.

@humitos

I understand that nbsphinx is a plugin that runs inside the HTML builder (which is the first one in our steps) so, for this particular case it won't make any difference in memory usage, right?

nbsphinx provides a source parser and most of its work is done during parsing and not while the builder is running.
Ideally, all Sphinx calls after the first one should completely skip the source parsing stage.

@mgeier mgeier changed the title Is it possible to control the "force" resp. "-E" option for Sphinx? Is it possible to control the "force" resp. "-E" option for Sphinx? (turns out it's actually the "-d" option) Jul 26, 2018
@stsewd
Copy link
Member

stsewd commented Jul 26, 2018

oh, I see, I think rtd have separate build dirs to be able to copy each generated resource in a clean way, but I'm not sure if that is really necessary, maybe we can reuse the same directory.

@mgeier
Copy link
Contributor Author

mgeier commented Jul 26, 2018

@stsewd Please note that the "environment" directory (given with the -d option, e.g. _build/doctrees-json) is different from the "output" directory (given as the second non-option argument, e.g. _build/json).

I guess it makes sense to have separate "output" directories for the builders in order to easily copy files around, but I don't see why it would make sense to have separate "environment" directories.

@agjohnson agjohnson added Needed: design decision A core team decision is required Improvement Minor improvement to code labels Aug 3, 2018
@humitos humitos added this to the Build stability milestone Aug 30, 2018
@humitos
Copy link
Member

humitos commented Aug 30, 2018

At first sight, it makes sense to me what you said regarding sharing the same -d PATH between different build steps (html, pdf, epub, etc) --note that json was removed and it's done inside the html step now.

  -d PATH           path for the cached environment and doctree files
                    (default: OUTPUTDIR/.doctrees)

I'd like to have more context on why this was done like this (independent environment for each step) and thinking a little more if it's something that can be really shared or it could cause some kind of conflict.

It seems it was using a shared direction for doctrees some years ago and it was changed here: a5d9d45

There is no much info there, maybe @ericholscher or @agjohnson can help us here. To me, it seems like a reasonable thing revert and use a shared path.

From the docs http://www.sphinx-doc.org/en/master/man/sphinx-build.html#cmdoption-sphinx-build-d

the doctrees can be shared between all builders

@agjohnson
Copy link
Contributor

I don't have any background on why this would have been changed. The change seems to imply we directly had some issues sharing the doctree environment though. If I had to guess, I'd say we're safe to reuse the doctree, though I think we don't want to reuse the existing doctree on the first build maybe?

I haven't dug too deep here, but it looks like we force a new doctree env on the first builder (unless it's the only builder?), and we don't force on subsequent build steps, though we are using one-off doctree envs.

It seems like what we want is to always wipe the doctree env and share that between builds instead? If there isn't a good reason to separate the doctree envs, I think this makes sense.

@ericholscher
Copy link
Member

Yea, I don't 100% remember why this was done. I'm guessing because of build caching of changed files. So:

  • I build the HTML files, and it updates all changes files
  • I build the PDF, but it doesn't see any files as changed since the "last" build, so it doesn't update anything

I'd be curious to see benchmarks here, and some real testing to see if this is an issue while running. I'm guessing it will lead to issues.

@humitos
Copy link
Member

humitos commented Sep 4, 2018

I build the PDF, but it doesn't see any files as changed since the "last" build, so it doesn't update anything

I don't think this will happen because the documentation says that it can be shared among all the builders. Although, it definitely worth a test.

I'd be curious to see benchmarks here, and some real testing to see if this is an issue while running. I'm guessing it will lead to issues.

This is also that needs some testing and could help us to decide what to do here.

I wouldn't take any action before having at least a couple of tests shared here for different projects. Otherwise, we will end up with strange behavior on production and we won't know why.

@agjohnson
Copy link
Contributor

This is perhaps a good candidate for a feature flag. We can test a small sample with this change. It is probably a slight speed up for builds, so might be a good addition. I'm putting this a few versions out though, it probably isn't a pressing priority right now.

If we notice build bugs, we can re-evaluate priority.

@agjohnson agjohnson modified the milestones: Build stability, 3.0 Sep 19, 2018
@agjohnson agjohnson added the Accepted Accepted issue on our roadmap label Sep 19, 2018
@stsewd
Copy link
Member

stsewd commented Oct 24, 2018

Just putting some tests here

Separated doctrees

sphinx-build -T -b html -d _build/doctrees . _build/html  9.01s user 0.30s system 72% cpu 12.873 total
sphinx-build -T -b singlehtml -d _build/doctrees-singlehtml .   6.21s user 0.28s system 67% cpu 9.614 total

sphinx-build -T -b html -d _build/doctrees . _build/html  13.76s user 0.44s system 82% cpu 17.216 total
sphinx-build -T -b singlehtml -d _build/doctrees-singlehtml .   9.62s user 0.42s system 65% cpu 15.374 total

sphinx-build -T -b html -d _build/doctrees . _build/html  9.90s user 0.32s system 73% cpu 13.818 total
sphinx-build -T -b singlehtml -d _build/doctrees-singlehtml .   6.16s user 0.28s system 65% cpu 9.828 total

sphinx-build -T -b html -d _build/doctrees . _build/html  13.36s user 0.35s system 79% cpu 17.183 total
sphinx-build -T -b singlehtml -d _build/doctrees-singlehtml .   8.08s user 0.40s system 59% cpu 14.348 total

sphinx-build -T -b html -d _build/doctrees . _build/html  9.48s user 0.33s system 78% cpu 12.575 total
sphinx-build -T -b singlehtml -d _build/doctrees-singlehtml .   6.28s user 0.26s system 71% cpu 9.192 total

Shared doctrees

sphinx-build -T -b html -d _build/doctrees . _build/html  9.96s user 0.31s system 78% cpu 13.104 total
sphinx-build -T -b singlehtml -d _build/doctrees . _build/singlehtml  3.34s user 0.24s system 99% cpu 3.615 total

sphinx-build -T -b html -d _build/doctrees . _build/html  8.96s user 0.33s system 70% cpu 13.119 total
sphinx-build -T -b singlehtml -d _build/doctrees . _build/singlehtml  3.42s user 0.20s system 99% cpu 3.625 total

sphinx-build -T -b html -d _build/doctrees . _build/html  11.01s user 0.36s system 79% cpu 14.351 total
sphinx-build -T -b singlehtml -d _build/doctrees . _build/singlehtml  3.41s user 0.23s system 99% cpu 3.652 total

@humitos
Copy link
Member

humitos commented Nov 14, 2018

This is perhaps a good candidate for a feature flag. We can test a small sample with this change. It is probably a slight speed up for builds, so might be a good addition

👍 we should go in this direction that is not harmful.

@humitos humitos removed the Needed: design decision A core team decision is required label Nov 14, 2018
@ericholscher
Copy link
Member

I'm curious if the projects in prod are still active. We can probably just ping each 4 of them and ask them to configure formats, and be done with it.

@stsewd
Copy link
Member

stsewd commented Nov 15, 2018

@ericholscher wrong issue I guess p:

@ericholscher
Copy link
Member

Ha yea, I was wondering where that comment went :D

@agjohnson agjohnson removed this from the 3.0 milestone Jan 25, 2019
@agjohnson agjohnson modified the milestones: 3.2, 3.3 Jan 25, 2019
@stsewd stsewd self-assigned this Jan 28, 2019
@stsewd stsewd modified the milestones: 3.3, 3.4 Mar 6, 2019
stsewd added a commit to stsewd/readthedocs.org that referenced this issue Mar 6, 2019
We are reducing build time with this.
And also saving a little of space in the builders.

According to sphinx, we are safe doing this:

> the doctrees can be shared between all builders

http://www.sphinx-doc.org/en/master/man/sphinx-build.html#cmdoption-sphinx-build-d

Closes readthedocs#4363
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Projects
None yet
Development

No branches or pull requests

5 participants