-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Design doc: new builder with explicit options #8103
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
Conversation
ed129aa
to
6e012d0
Compare
6e012d0
to
2e5c156
Compare
2e5c156
to
29ba47a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good start to me! There is a lot happening here, tho 😄
My notes:
- it seems we are changing completely where we put the effort to onboard people. Moving it from "magic" to "documentation guides"
- I'm fine having guides explaining how to do things, but I'd expect the user to have a nice experience with our platform without having to read them before importing the project
- without "the guessing magic", how do we expect to be the first build? will it succeed without reading all of those guides?
- how we will point the user to the correct problem and guide them towards the solution from the build detail's page when the first build fails?
- as a user, importing a project and seeing it immediately failing is terrible --we would need a super nice and decorated page explaining all the problems together with the solution at once
- failing the first build because
.readthedocs.yaml
does not exist, and then failing becausepython.sphinx.conf
is not defined, and then becauseSphinx
is not in therequirements.txt
, and then because another and another (over and over) config was mandatory and wasn't defined is not a good experience at all - having a good "Getting Starting with Read the Docs" document with all the sane config we expect from the user being copy&past-able would help us to reduce the on-boarding barrier
- showing copy&past-able messages from build detail's page on failed builds will also help to improve the experience
I also think we want to guide users towards a "minimum level of docs quality" where all the docs hosted at Read the Docs have all of these features, and readers would expect them to exist (unless the author decided to disable them). This was my thought when I suggested #7870. In my head, using Read the Docs should automatically bump your documentation quality to the next level.
I think that everything that I'm saying here could be achieved with:
- sane default to make everything to Just Work ™️ as much as we can
- make those defaults easily override-able / disabled completely
- improve the on-boarding UX and failed builds UX
- pre-install packages we control (or make suggestions to install them)
Summarizing, I'm fine removing as much magic as we can, but we need to know that this magic helps a lot of new users to have a better on-boarding. However, it generates conflicts to more experienced users. We need to find a sane balance where both type of users can co-exist. Removing all of it expecting the users to read all our guides is not the answer here without working on UX, to me.
The new Python environments would install only the latest versions of the following dependencies: | ||
|
||
Virtualenv | ||
- Sphinx or MkDocs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of installing Sphinx/MkDocs here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So users don't need to have a requirements file for this, and installing them with pip doesn't have the same problems as Conda, they can be easily overridden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conflicts in some way with the confusions listed in this document:
The Sphinx version (or from any other tool) isn't the one they expect.
The new builder requires a conf.py
. So, they will need to have Sphinx installed locally and they have ran sphinx-quickstart
. Then, when importing on Read the Docs, we are going to install a random version (potentially different than the one they have installed locally). In the end, we are not fixing this problem / removing this confusion / making this confusion clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another things that rings me a bell here are:
- we will need to deal with not installing Sphinx/MkDocs when we support new different doc tools (eg. Hugo, etc)
- Read the Docs behaves differently when using conda than when using pip. I don't think we have a good reason to explain that to users: "conda resolver is broken" is not a good one 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they don't have a requirements file, they aren't expecting any version at all. If they have sphinx==1.2.3
or sphinx
(latest) and we install something different or isn't downgraded than that's a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say we should install only dependencies that are required at build time to make Read the Docs' features to work: readthedocs-sphinx-ext
. All the other extra dependencies should be defined by the user. We can start with 0 extra dependencies, and add them later if we find they are required, but at this point, I wouldn't include them here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I assume readthedocs-sphinx-ext
depends on Sphinx, but we can probably install it without.
I think in this builder we should not install sphinx or mkdocs, and require the user to understand what is happening. Hopefully they can eventually specify it easily in the rtd.yaml (install: sphinx
) or similar, but I agree that having less magic to manage is good.
We should pass the minimal needed information into the context. | ||
This is related to :doc:`/development/design/theme-context`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like a good moment to implement the new theme context together with the new builder 👍 --whatever its structure would be, but having it under a namespace (readthedocs.v1.*
) is key to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed -- though we should also heavily minimize the data to start, treating it basically as an API 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that this is not decided yet. The document proposes to remove (if possible) passing html_context
or at least reduce it.
Considering that we are trying to avoid this magic, I'd propose to do a little research to understand if it's possible to not touch this variable from our side and remove it completely since it's a Sphinx-only feature. Also, because we are trying to remove ourselves from the build process.
(Note: I'm considering using an intermediate tool-agnostic metadata.yaml
file for required context like this in a future builder)
(analytics, canonical URL, etc), we should write a guide instead of applying our magic. | ||
|
||
The extension also injects some custom js and css files. | ||
We could try another more general approach and inject these on each HTML file after the build is complete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach makes sense if we are going to support other building tools in the future. This will probably have some impact on big projects with many HTML files, since we will need to rewrite all of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I think adjusting the built HTML is the right way to handle this, so we can support non-Sphinx builders. In mkdocs we're just adding this to their setting:
readthedocs.org/readthedocs/doc_builder/backends/mkdocs.py
Lines 175 to 178 in 924699c
user_config.setdefault('extra_javascript', []).extend( | |
[js for js in extra_javascript_list if js not in user_config.get( | |
'extra_javascript')] | |
) |
But it seems like explicitly doing it after build against all HTML is more extensible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking it out from the extension into the build process is probably correct here. 👍
Both builders create an ``index.{rst,md}`` file with some suggestions if one doesn't exist, | ||
this is to avoid surprises to users, but sometimes this is done on purpose. | ||
We could change our default 404 page to include some of these suggestions instead. | ||
Related to `#1800`_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build should fail, IMO. We should show the problem in the build detail's page instead in a 404 page. We already known that the build is not in a good state and we should communicate that to the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, fail the build.
- Latex settings | ||
(they are used to improve the output for Chinese and Japanese languages, we should write a guide instead) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is related to the sane defaults we talked about. I think this provides value to non-experienced users and we haven't had troubles with it. Also, this is one of the cases where an experienced user can override these configs easily. I'd like to have more like this one instead of less ones 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have mixed feelings here. If we're doing almost nothing in our conf.py override, I do think there's value in trying to get that to nothing at all.
If the goal here is giving users options here and letting them configure it with guides, I think that is a reasonable solution for most cases, but we need to figure out where the line is.
I think getting to the point where we don't override the users conf.py at all, if possible, would be a huge win for simplifying our builds. I don't know if it's realistic though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After talking to @agjohnson I changed my mind here. I prefer to do nothing in the config file and write a good guide with a copy&paste-ready code explaining this.
I think getting to the point where we don't override the users conf.py at all, if possible, would be a huge win for simplifying our builds. I don't know if it's realistic though.
Agree. This is where I'd like to end up as well after the new builder.
There are some things our extension does that are already supported by Sphinx or themes | ||
(analytics, canonical URL, etc), we should write a guide instead of applying our magic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the kind of the features that "bump your documentation to the next quality level" just because you are using Read the Docs.
You don't have to worry about dealing with Google Analytics code, canonical URLs, sitemap, robots, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Analytics is already an opt-in option, so I'd be ok with moving the canonical url to an option too (we hit problems with injecting the canonical url twice). I'm +1 on giving users more control over this kind of settings instead of removing them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine moving them to options and I'd like to see them built outside the extension as a post-processing HTML feature instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good start. We definitely need to think a bit more about the migration and final state we're hoping to get to.
(analytics, canonical URL, etc), we should write a guide instead of applying our magic. | ||
|
||
The extension also injects some custom js and css files. | ||
We could try another more general approach and inject these on each HTML file after the build is complete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I think adjusting the built HTML is the right way to handle this, so we can support non-Sphinx builders. In mkdocs we're just adding this to their setting:
readthedocs.org/readthedocs/doc_builder/backends/mkdocs.py
Lines 175 to 178 in 924699c
user_config.setdefault('extra_javascript', []).extend( | |
[js for js in extra_javascript_list if js not in user_config.get( | |
'extra_javascript')] | |
) |
But it seems like explicitly doing it after build against all HTML is more extensible.
|
||
The extension injects the warning banner if you are reading the docs from a pull request. | ||
We could implement this like the version warning banner instead, so it works for MkDocs too. | ||
(this is injected with the version selector). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if it makes sense to migrate the version warning banner to the new builder. We have had a lot of different opinions about how it should work and we ended up creating a Sphinx extension that allow users to customize different aspects of it.
I'd say that it would be better to remove it completely and either:
- maintain the extension we already have --inviting mkdocs (and other doctools) community to build their own extension for this, or
- implement the JS version at Read the Docs that allow customization (text, link to "latest" version, etc)
I think 1) is better if we want to go in the "provide integration points where the community can hook into". Besides, it's less work for us 😄
- Latex settings | ||
(they are used to improve the output for Chinese and Japanese languages, we should write a guide instead) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have mixed feelings here. If we're doing almost nothing in our conf.py override, I do think there's value in trying to get that to nothing at all.
If the goal here is giving users options here and letting them configure it with guides, I think that is a reasonable solution for most cases, but we need to figure out where the line is.
I think getting to the point where we don't override the users conf.py at all, if possible, would be a huge win for simplifying our builds. I don't know if it's realistic though.
.. _#4827: https://github.com/readthedocs/readthedocs.org/issues/4827 | ||
.. _#4820: https://github.com/readthedocs/readthedocs.org/issues/4820 | ||
|
||
Web settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we get rid of web settings entirely for things that are in the config file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they are still useful when you want to build from an existing release, since you can't change that code to add a config file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should remove them and provide a nice way to support version aliases. So, in case you have to re-build an old tag (e.g. 2.1
) you can create a new branch/tag (e.g.2.1.1
) and keep the same URL and everything but with updated/fixed docs by using an alias for that new branch/tag.
In order to simplify our code and have all projects using the same options and dependencies | ||
we want to fully migrate all projects to use the new builders. | ||
We could put a date to do this, and contact all users of old projects about this change | ||
(an entry in our blog would also be nice). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section could probably use a bit more detail. Migrating all our users is going to be a lot of work. There's a number of things we could do to help them migrate though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't sure how to expand this, I have updated it to put a list of things that could be done.
I do wonder about implemented this given the change to the YAML file. I think that seems like a natural chance to break compatibility. @stsewd what needs to happen to unblock this? I believe @humitos had some thoughts here, but I'm 👍 on moving forward with a very simple builder for |
@ericholscher I guess all the conversations that aren't marked as resolved still need some decisions, but I put on the doc that we can implement this new builder under a feature flag, so we don't need to implement/decide everything in one single iteration/PR. |
@stsewd gotcha -- then I think we can move forward towards implementation then, unless anyone objects. I'm 👍 on it. |
These new builders/environments would be under a feature flag. | ||
We can keep the implementation incrementally by start using a feature flag on some of our projects first, | ||
after we everything is implemented we can move the flag to be active for projects created after ``x`` date, | ||
and past projects would use the old ones. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest making this new builder opt-in and use build.builder: 2
instead of a feature flag. I'm not sure we can deprecate and remove the builder that we currently have. I think it's a good product for .com users.
I had a long conversation with Anthony last week about "the new builder" and I considered this PR as the next step in my proposal before reaching the final goal (*). So, I was 👍🏼 moving forward here. However, @agjohnson ask me if it may not make sense to move directly to the final step without this intermediate builder to avoid maintaining 3 builders in the future. Now, I'm not yet sure what's the best next step. I suggested Anthony talk to you @ericholscher explaining what we talked about before moving forward in case it helps to reduce the amount of work required here. In any case, for this PR I'd expect the following before start implementing it:
I'm happy to keep talking about this if needed. I think we should have very clear what's the final goal related to executing custom commands (via (*) the final goal is to not require control on the build process (dependencies, commands, etc): RTD would only work over the generated HTML + expected |
Maybe the thing that makes the most sense is just building a "v2" builder that literally does nothing other than run the sphinx & mkdocs commands, then we can build on top of that over time, but users who want that functionality can opt into it? I agree though, if we end up allowing custom build commands, at some point this isn't even a builder, it's just a thing that calls the users command and post-processes output. Maybe we should start with that and see how it works? |
I'm fine creating a new builder that does nothing (do not install extra dependencies) if we can decouple as much as we can the work we do in I want to make sure that we can guarantee ourselves forward compatibility with the future changes to reach our goal. However, I don't see too much value right now by having a builder that does nothing but doesn't add functionality either without having a clear path and an idea for a contract defined. It's a good intermediate step towards the final goal, though. This is the main reason why I think it worth to keep discussing a little more before proceed building this and then realizing that we still have some magic in our build process that users depend on. |
This document is a continuation of Santos' work about "Explicit Builders" (see decisions about the final goal, proposing a clear direction to move forward with intermediate steps keeping backward and forward compatibility. Most of this is a dump of my talk to Anthony about these ideas and some extra work done on the whiteboard after receiving some feedback from him. The most important feedback here would be to know if this is a place where we would like to be sooner than later and in that case, make sure that what we are deciding and building on #8103 is compatible with the direcion and final goal proposed here.
Right, this work is required to "do nothing" and post-process the output. We would hopefully remove all tool-specific integration on our side, if possible. Presumably passing in environment variables instead of editing The hardest thing here is probably search, where we're injecting code to get data out of the Sphinx internals. We'll need to decide if that is worth the complexity, or move that code into something users can enable if they want as a third-party extension. (eg.
Agreed. We should be clear on what those future goals are, and design for them when we're building the system.
Agreed. I think the biggest question in my mind is around allowing users to customize the build commands -- it seems we want to get there eventually but we aren't ready. However, we could pretty easily build a system that allows customization of a subset of commands while still being able to eventually support that. My thinking:
The v1 of this plan is basically what we're discussing in this PR, but we should have a good idea of what v2 might look like, so we can design it properly. |
@ericholscher All you said makes sense to me 👍🏼 I went ahead today's morning and wrote in a document (see #8190) all I had in mind about this topic, what I talked to Anthony and consider Santos' PR as a good intermediate step, and also the roadmap conversations we already had. It's not ready for review yet, just a way to early communicate my concern about a clear path and how to make the correct steps in that direction. I don't want to deviate the conversation we are having here to a new PR, but use the new PR as a proposal for a final goal to make the required changes on this PR to reach that goal --avoiding being blocked later due to a dependency that we cannot remove. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Closing this PR as we already took the ideas from here and we are moving in a different direction. See #8190 for more information. Please, if you think there are still good ideas to implement here, open a new issue or comment into the new issues that are tracking |
I'm suggesting to remove everything basically :D maybe is it too much? Let me know what you think.
Some "big" decisions are:
html_context
?Find more while reading the document!
Rendered version: https://docs--8103.org.readthedocs.build/en/8103/development/design/explicit-builders.html