potiuk commented on code in PR #73477:
URL: https://github.com/apache/airflow/pull/73477#discussion_r4068627780
##########
devel-common/src/docs/build_docs.py:
##########
@@ -693,52 +729,19 @@ def build_docs(
all_spelling_errors.update(package_spelling_errors)
if not one_pass_only:
- # Build documentation for some packages again if it can help them.
- package_build_errors = retry_building_docs_if_needed(
- all_build_errors=all_build_errors,
- all_spelling_errors=all_spelling_errors,
- autobuild=autobuild,
- docs_only=docs_only,
- jobs=jobs,
- verbose=verbose,
- package_build_errors=package_build_errors,
- originally_built_packages=packages_to_build,
- # If spellchecking fails, we need to rebuild all packages first in
case some references
- # are broken between packages
- rebuild_all_packages=spellcheck_only,
Review Comment:
This is what resulted in (infrequent) long spellcheck builds in edge
cases,because when it failed once during spellchecking, you had to rebuild it
**again** and this time all packages and not "spellcheck_only", because you did
not know which package needs rebuilding to re-create inventory used.
Spellcheck builds do **not** recreate inventories. This is the problem. So
if you run "spellcheck-only" build and it fails because of missing new method
in another package + unrelated spellcheck errors, you need to effectively
disable "spellcheck-only" and attempt to build full inventoris of all the
packages you build - to avoid misleading errors- otherwise you will never have
the inventory created with the new methods and most of the things you will see
are those (seemingly) missing methods - while spellcheck or other genuine
documentation errors (too short header line for example) that are really
causing it will be deeply buried.
At this point you are also really guessing it - because it might also be
genuine documentation errors not only inventory issue (for example typo). You
really genuinly do not know here. Note - we **only** attempt that "rebuild"
when there are errors indicating that **likely** there are new objects that
need new inventory (There are few types of error that indicate that this might
be the case). Some errors will just not be retried - the "rebuild if needed"
checks the errors and will not attempt to rebuild if errors are of a different
kind - it will rebuild only if all errors are "missing thing".
That **migh** result in some cases indeed the 4 th build was needed to get
the docs green and you need switch from doing spellcheck only - because you
need to rebuild inventories.
Imagine the case A -> B -> C (i.e. package C adding new thing, used by B and
package A using new thing added in B). An example (realistic and sometimes
happening) case would be (but this is extremely rare though) - adding common
provider method used by SSH provider that adds new method that is used by SFTP
provider. Yes. Happened.
This led to 4 (!) builds needed - one spellcheck only, and 3 doc +
spellcheck builds:
1) spellcheck build -> failing (and you guess - because of new method added).
2) doc + spellcheck build of everything to rebuild inventory -> failing A,B
if they were built before C . For spelcheck we need to build **all** packages
not only the failing one, because spellcheck build does not produce new
inventory, it only does spellcheck, so we also need to build C, to get the
inventory with new methods.And this needs to be really "all packages" build
here in case of spellcheck because we need to have all inventories locally -
even if some of the packages failed. For example even if C spellcheck worked in
1) - there is missing inventory for C. So we really need to build
**everything** here.
3) repeatebuild of A, B (those two failed. if A is build before B it will
fail again. Again that one needs to be "full" build because we need also B
inventory to be build.
4) build of A.
We could avoid 4 if 3) is done sequentially and if we build it in the right
sequence B->A. But for that we would need to sort the remaining builds
topologically.
Another option in this case is to switch such PR that adds several things
into series of PRs where you knowingly add things in the right order. But that
is kinda superpower to know that this is the case and figure out the right
sequence and split the PR in the right way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
