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

Share doctree between builders #5407

Merged
merged 3 commits into from
Mar 12, 2019

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented 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 #4363

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
@stsewd stsewd requested a review from a team March 6, 2019 17:10
@stsewd
Copy link
Member Author

stsewd commented Mar 6, 2019

Tested this in the local instance inside docker, I don't see much improvements in the time like it did outside (#4363 (comment))

But the obvious improvement is the size.

@agjohnson
Copy link
Contributor

Perhaps this would make sense under a feature flag. I feel like none of us remember why we had to separate the doctree in the first place, I'm guessing we'll find out really quick 🙂


FEATURES = (
(USE_SPHINX_LATEST, _('Use latest version of Sphinx')),
(USE_SETUPTOOLS_LATEST, _('Use latest version of setuptools')),
(ALLOW_DEPRECATED_WEBHOOKS, _('Allow deprecated webhook views')),
(PIP_ALWAYS_UPGRADE, _('Always run pip install --upgrade')),
(SKIP_SUBMODULES, _('Skip git submodule checkout')), (
(SKIP_SUBMODULES, _('Skip git submodule checkout')),
(
Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry but it was bothering me seeing this js kind of indentation p:

Copy link
Member

Choose a reason for hiding this comment

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

Is this change made by pre-commit? If you did it manually, it will be reverted in the next pass otherwise.

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 did it manually, but I run precommit on this file, and it didn't change anything

@stsewd
Copy link
Member Author

stsewd commented Mar 11, 2019

@agjohnson feature flag added

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

The PR looks very simple to me and accurate.

I like the idea of having it under a Feature flag. We need to remember to add the flag and sample some projects after deploying it.

@humitos
Copy link
Member

humitos commented Mar 12, 2019

BTW, shouldn't we update this docs https://docs.readthedocs.io/en/latest/guides/feature-flags.html ?

@stsewd
Copy link
Member Author

stsewd commented Mar 12, 2019

BTW, shouldn't we update this docs https://docs.readthedocs.io/en/latest/guides/feature-flags.html ?

I don't think so, we are just testing this to then remove it, and this shouldn't affect users at all if what we are doing is ok. So, feature flag will be removed soon if things go well :)

@stsewd
Copy link
Member Author

stsewd commented Mar 12, 2019

I'm adding a trello card to keep track of adding this flag to some projects

@stsewd stsewd merged commit b44dab4 into readthedocs:master Mar 12, 2019
@stsewd stsewd deleted the share-doctree-between-builders branch March 12, 2019 16:12
@ericholscher
Copy link
Member

Pretty sure this is going to blow up in production. We can have multiple projects running on the same builder across different versions, which will cause weird race conditions from this I'm 99% sure.

@stsewd
Copy link
Member Author

stsewd commented Mar 18, 2019

But the _build/ folder is created for each version, we can only have race conditions if we trigger a build to the same version twice, the same race condition would be seen before this change.

@ericholscher
Copy link
Member

Ah right, it must have been another issue then. Guess we'll see in prod as Anthony said.

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.

4 participants