Build: allow partial override of build steps #11710

stsewd · 2024-10-23T21:21:34Z

I was replacing some of the config models with dataclasses, but I found myself re-implementing some helpers that pydantic provides, so I'm introducing that new dep, it has everything we need, we may be able to use it to simplify our validation once all models are migrated to pydantic.

About incompatible options

I decided to just not allow formats when build.jobs.build is used, seems just easier that way. But not sure if users may want to just override the html build step while still using the default build pdf step, if that's the case, we may need to support using formats... or have another way of saying "use the default".

build.jobs.create_environment is kind of incompatible with python/conda.
Since users will need to manually create the environment,
but they may still want to use the defaults from build.jobs.install,
or maybe they can't even use the defaults, as they are really tied to the default create_environment step.

Which bring us to the next point,
if build.jobs.create_environment is overridden,
users will likely need to override build.jobs.install and build.jobs.build as well...
Maybe just save the user some time and require them to override all of them?
Or maybe just let them figure it out by themselves with the errors?
We could gather more information once we see some real usage of this...

Override them all

I chose to make build.html required if users override that step, seems logical to do so.

Do we want to allow users to build all formats? I'm starting with html and pdf, that seem the most used. But shouldn't be a problem to support all of them. I guess my only question would be about naming, htmlzip has always been a weird name, maybe just zip? I just went ahead and allowed all, with the same name as formats.

Docs

I didn't add docs yet because there is the question... should we just expose this to users? Or maybe just test it internally for now?

Closes #11551

stsewd · 2024-10-24T19:44:40Z

Checks are failing because we have their deps hardcoded

https://github.com/readthedocs/common/blob/b8051be8f8221f5fe448dc81e1f2c7668090a732/pre-commit-config.yaml#L111-L113

They should pass after the PR is merged.

ericholscher

This looks like a reasonably small set of changes overall compared to what I might have been worried about. Definitely seems nice introducing pydantic to have proper models in the config classes.

readthedocs/core/utils/objects.py

readthedocs/rtd_tests/fixtures/spec/v2/schema.json

humitos · 2024-10-29T10:09:58Z

I decided to just not allow formats when build.jobs.build is used, seems just easier that way. But not sure if users may want to just override the html build step while still using the default build pdf step, if that's the case, we may need to support using formats... or have another way of saying "use the default".

I think formats: [html, pdf] and build.jobs.build.html override would be a common/valid use case, and I say we should support it. I'm fine not supporting in the first iteration if it's complicated to do it, but we should consider doing it after this first iteration is done.

What is the complexity you found while working on this?

build.jobs.create_environment is kind of incompatible with python/conda. Since users will need to manually create the environment, but they may still want to use the defaults from build.jobs.install, or maybe they can't even use the defaults, as they are really tied to the default create_environment step.
Which bring us to the next point, if build.jobs.create_environment is overridden, users will likely need to override build.jobs.install and build.jobs.build as well... Maybe just save the user some time and require them to override all of them? Or maybe just let them figure it out by themselves with the errors? We could gather more information once we see some real usage of this...

I wouldn't start by adding all this complexity to the validation. If users override those steps but they follow what we expect (our contract) that seems fine.

I didn't add docs yet because there is the question... should we just expose this to users? Or maybe just test it internally for now?

I'm fine start by testing it internally first. We have a few issues from users where we can suggest them to try this approach and receive feedback from them to work in the next iteration.

humitos

This work looks good to me.

It seems there is no need to apply some of the restrictions implemented in this PR, tho. I left some suggestions that allow using formats together with build.jobs.build overrides.

I also think we should remove the restriction to always override build.jobs.build.html. I didn't see anything blocking us to allow that.

humitos · 2024-10-29T10:26:02Z

readthedocs/config/config.py

+        if not "html" in build_jobs_build:
+            raise ConfigError(message_id=ConfigError.HTML_BUILD_STEP_REQUIRED)


I think build.jobs.build.html shouldn't be mandatory. That means that users cannot override only any of the other formats like build.jobs.build.pdf or build.jobs.build.epub.

humitos · 2024-10-29T10:28:53Z

readthedocs/config/config.py

@@ -692,6 +727,15 @@ def validate_search(self):

        return search

+    def validate_incompatible_keys(self):
+        # `formats` and `build.jobs.build.*` can't be used together.


I would remove this restriction. It seems pretty common to use the default PDF builder (via formats: [pdf]) , but override build.jobs.build.html. What is the problem this restriction solves?

humitos · 2024-10-29T10:36:10Z

readthedocs/doc_builder/director.py

+        build_overridden = config.build.jobs.build is not None
+        if build_overridden:
+            self.run_build_job("build.html")
+            self.run_build_job("build.htmlzip")
+            self.run_build_job("build.pdf")
+            self.run_build_job("build.epub")
+        else:
+            self.build_html()
+            self.build_htmlzip()
+            self.build_pdf()
+            self.build_epub()


Here is where we can remove the formats restriction:

for format in ["html", "htmlzip", "pdf", "epub"]: commands = get_dotted_attribute(self.data.config, f"build.jobs.build.{format}", []) if commands: self.run_build_job(f"build.{format}") else: method = getattr(self, f"build_{format}") method()

That allows users to override only the format they want to override while keeping the default commands when using formats as well.

stsewd · 2024-10-29T16:14:27Z

Supporting formats and build.jobs.build.* will require users to specify the format in both places when overriding, and both places must match. This is:

formats: ["pdf"]
build:
  # pdf is duplicated twice
  pdf: echo pdf
  # This is invalid, as epub isn't in formats.
  epub: echo epub

It just feels more explicit having everything in one place, and not depend on other keys. Also, as mentioned in the other issue, overriding one step may require to update all build steps, which will make it not so common to re-use the default commands.

humitos · 2024-10-30T11:22:44Z

Supporting formats and build.jobs.build.* will require users to specify the format in both places when overriding, and both places must match

I don't see a problem in the example you shared and I don't think the build.epub key will be invalid there. In that example, the build will end up with PDF and ePUB formats built without issues. Also,

formats: ["epub"]
build:
  pdf:
    - echo pdf

Is also valid and will use the default command for ePUB and a custom one for PDF.

I don't think we need to perform any kind of validation.

ericholscher · 2024-10-30T17:25:41Z

We agreed to move forward with:

Requiring formats key for building that step
If the user overrides the build job format build.jobs.build.pdf, it has to be in the format key.
The default step will be run if it isn't overridden.

stsewd · 2024-11-04T16:03:50Z

We also noted that running a default step is only required for sphinx, mkdocs and other tools don't have a default step for pdf/zip/epub.

ericholscher · 2024-11-04T18:24:26Z

My only question is what to do with html. We don't currently allow it in formats, and adding it also makes it complex, because it will change the meaning of previous configs where it wasn't defined but still got built. Making it required makes sense to me in build.jobs.build.html, but then we don't have a way to specify "Use the default html build" 🙃

For example, does this config build html?

formats: ["pdf"]
build:
  pdf:
    - echo pdf

This one does:

build:
  html:
    - $RTD_DEFAULT_BUILD
  pdf:
    - echo pdf

This one doesn't:

build:
  pdf:
    - echo pdf

But it creates a lot of ambiguity.

So I do wonder if the current implementation is the best, where we disallow formats & build.jobs.build.*, and then give an explicit way to run default commands?

The other way to think about this would be using the approach we discussed, and require HTML, so maybe we just say html is always built, and is implied in formats?

So to build default HTML:

formats: ["pdf"]
build:
  pdf:
    - echo pdf

And to override HTML:

formats: ["pdf"]
build:
  html:
    - echo html
  pdf:
    - echo pdf

ericholscher · 2024-11-04T18:28:24Z

Thinking about this more -- I think the later option still makes sense. We just always assume HTML is built. I don't think there's a super valid use case for not building HTML (though something like Django's use of our platform might be a good example, though they still link to our HTML build when their docs are down as a mirror)

stsewd · 2024-11-04T21:18:07Z

I guess your examples are for sphinx. But what about mkdocs and other tools?

We don't have a default build command for PDF.

formats: ["pdf"]
build:
   html: echo html

Should this config be invalid for non-sphinx configs?

Also, mkdocs does have a default commnad for html, so for mkdocs not specifying html should be valid I guess

ericholscher · 2024-11-04T23:04:19Z

Yea other tools presumably dont have default commands for non-html builds. They should be able to override those formats though...

humitos · 2024-11-05T10:24:51Z

I think the later option still makes sense. We just always assume HTML is built

This is how Read the Docs currently works, and I don't think we have to change that.

You later option from #11710 (comment) makes sense to me.

ericholscher · 2024-11-11T21:11:04Z

I think the plan here is as I outlined above, and we should be able to start moving forward on this:

So to build default HTML:

formats: ["pdf"]
build:
  pdf:
    - echo pdf

And to override HTML:

formats: ["pdf"]
build:
  html:
    - echo html
  pdf:
    - echo pdf

@stsewd I think shouldn't be a huge change, and we start moving forward with testing here.

stsewd · 2024-11-11T22:15:35Z

I think the plan here is as I outlined above, and we should be able to start moving forward on this:
So to build default HTML:
formats: ["pdf"]
build:
  pdf:
    - echo pdf
And to override HTML:
formats: ["pdf"]
build:
  html:
    - echo html
  pdf:
    - echo pdf
@stsewd I think shouldn't be a huge change, and we start moving forward with testing here.

There's still the question about what to do with mkdocs, as it doesn't have default commands for other than HTML

ericholscher · 2024-11-11T22:23:20Z

@stsewd #11710 (comment)

ericholscher

This looks good to me overall. I think deploying it and testing it on test-builds before documenting is fine with me, so we can double check we're happy with the semantics.

cthoyt · 2024-11-20T23:29:17Z

@ericholscher I am happy to provide external feedback when this is ready - I already have a use case in mind discussed in #11766. Please ping me when this is deployed if you'd like an external tester! Cheers and thanks for the great work :)

humitos

This looks good to me, with a small change in the implementation to make it easier to follow the workflow when commands are overridden.

readthedocs/doc_builder/director.py

miroi · 2024-11-25T19:33:58Z

Thanks for the merge !

Please when will it be active in https://readthedocs.org/dashboard/ ?

stsewd · 2024-11-25T22:31:02Z

Thanks for the merge !

Please when will it be active in https://readthedocs.org/dashboard/ ?

It should be live by tomorrow in the afternoon.

- This makes it consistent with other dev deps. - Uses partial jobs override introduced in readthedocs/readthedocs.org#11710

stsewd added 8 commits October 23, 2024 16:21

Build: allow partial override of build steps

3b947be

Fix tests

004a3aa

Update schema

3a61248

Merge branch 'main' into build-override

39d25e5

Tests

6228aeb

Tests

6cd3ad7

Test empty overrides

0034f53

Maybe is tox?

8cc5421

stsewd marked this pull request as ready for review October 24, 2024 19:44

stsewd requested a review from a team as a code owner October 24, 2024 19:44

stsewd requested a review from ericholscher October 24, 2024 19:44

auto-assign bot assigned stsewd Oct 24, 2024

ericholscher reviewed Oct 24, 2024

View reviewed changes

readthedocs/core/utils/objects.py Show resolved Hide resolved

readthedocs/core/utils/objects.py Show resolved Hide resolved

readthedocs/core/utils/objects.py Outdated Show resolved Hide resolved

readthedocs/rtd_tests/fixtures/spec/v2/schema.json Show resolved Hide resolved

Updates from review

8e0c438

humitos reviewed Oct 29, 2024

View reviewed changes

humitos mentioned this pull request Nov 11, 2024

Docusaurus basics #11752

Merged

cthoyt mentioned this pull request Nov 14, 2024

Implement dependency groups cthoyt/cookiecutter-snekpack#32

Draft

stsewd added 6 commits November 18, 2024 18:22

Merge branch 'main' into build-override

82877fa

Allow formats with build.jobs.build.*

3ef8fe6

Merge branch 'main' into build-override

d4b53b8

Remove unused variable

5a8a52e

Format

890552d

Use old method for now

806ff86

stsewd requested review from humitos and ericholscher November 20, 2024 22:38

ericholscher approved these changes Nov 20, 2024

View reviewed changes

humitos reviewed Nov 21, 2024

View reviewed changes

readthedocs/doc_builder/director.py Outdated Show resolved Hide resolved

readthedocs/doc_builder/director.py Outdated Show resolved Hide resolved

stsewd added 5 commits November 25, 2024 11:46

Use conditional

0c5fea8

Move all actions to their method

c8cba44

Unused variable

c0b463c

Merge branch 'main' into build-override

1edef9c

Fix

f22413d

stsewd merged commit ebe3b2c into main Nov 25, 2024
4 of 5 checks passed

stsewd deleted the build-override branch November 25, 2024 17:50

miroi mentioned this pull request Nov 25, 2024

build.commands shall support custom package installation (build.apt_packages) #9599

Open

nijel added a commit to nijel/weblate that referenced this pull request Nov 26, 2024

docs: use dependency groups for deps

ecb8cc7

- This makes it consistent with other dev deps. - Uses partial jobs override introduced in readthedocs/readthedocs.org#11710

nijel added a commit to nijel/weblate that referenced this pull request Nov 26, 2024

docs: use dependency groups for deps

99da751

- This makes it consistent with other dev deps. - Uses partial jobs override introduced in readthedocs/readthedocs.org#11710

nijel added a commit to nijel/weblate that referenced this pull request Nov 26, 2024

docs: use dependency groups for deps

49d67da

- This makes it consistent with other dev deps. - Uses partial jobs override introduced in readthedocs/readthedocs.org#11710

nijel added a commit to nijel/weblate that referenced this pull request Nov 26, 2024

docs: use dependency groups for deps

fdb9302

- This makes it consistent with other dev deps. - Uses partial jobs override introduced in readthedocs/readthedocs.org#11710

nijel added a commit to nijel/weblate that referenced this pull request Nov 26, 2024

docs: use dependency groups for deps

5905b19

- This makes it consistent with other dev deps. - Uses partial jobs override introduced in readthedocs/readthedocs.org#11710

nijel added a commit to nijel/weblate that referenced this pull request Nov 26, 2024

docs: use dependency groups for deps

7507710

- This makes it consistent with other dev deps. - Uses partial jobs override introduced in readthedocs/readthedocs.org#11710

nijel added a commit to nijel/weblate that referenced this pull request Nov 27, 2024

docs: use dependency groups for deps

4982484

- This makes it consistent with other dev deps. - Uses partial jobs override introduced in readthedocs/readthedocs.org#11710

nijel added a commit to WeblateOrg/weblate that referenced this pull request Nov 27, 2024

docs: use dependency groups for deps

90cd712

- This makes it consistent with other dev deps. - Uses partial jobs override introduced in readthedocs/readthedocs.org#11710

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build: allow partial override of build steps #11710

Build: allow partial override of build steps #11710

		if not "html" in build_jobs_build:
		raise ConfigError(message_id=ConfigError.HTML_BUILD_STEP_REQUIRED)

Build: allow partial override of build steps #11710

Build: allow partial override of build steps #11710

Conversation

About incompatible options

Override them all

Docs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment