Add traceback information to exceptions during docbuild

[tobiasdiez]

The docbuild was failing for me, and it was hard to track down where the original error occurred because there was no (proper) traceback information. In this ticket, such a traceback information is added for exceptions in the docbuild worker following the approach taken in https://bugs.python.org/issue13831.

Example output:

sage_setup.docbuild.utils.RemoteException: 
"""
Traceback (most recent call last):
  File "/mnt/d/Programming/sage/src/sage_setup/docbuild/utils.py", line 179, in run_worker
    result = target(task)
  File "/mnt/d/Programming/sage/src/a_test_docbuild_exc.py", line 7, in target
    1 / 0
ZeroDivisionError: division by zero
"""

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/mnt/d/Programming/sage/src/a_test_docbuild_exc.py", line 12, in <module>
    build_many(target, range(8), processes=8)
  File "/mnt/d/Programming/sage/src/sage_setup/docbuild/utils.py", line 336, in build_many
    raise worker_exc.original_exception
ZeroDivisionError: division by zero

for the test file

from sage_setup.docbuild.utils import build_many

def target(N):
    import time, os, signal
    if N == 1:
        # Task 4 is a poison pill
        1 / 0
    else:
        time.sleep(0.5)
        print('Processed task %s' % N)

build_many(target, range(8), processes=8)

CC: @dimpase @jhpalmieri @seblabbe @slel @orlitzky

Component: documentation

Author: Tobias Diez

Branch/Commit: c37d055

Reviewer: Michael Orlitzky

Issue created by migration from https://trac.sagemath.org/ticket/31005

[tobiasdiez]

Author: Tobias Diez

[seblabbe]

comment:3

The patchbot reports failures for the files that are changed by the branch:

----------------------------------------------------------------------
sage -t --long --warn-long 40.9 --random-seed=0 src/sage_setup/docbuild/utils.py  # 1 doctest failed
sage -t --long --warn-long 40.9 --random-seed=0 src/sage_setup/docbuild/__init__.py  # 1 doctest failed
----------------------------------------------------------------------

To help the review, may you add an example in the description field of the kind of new information printed in the dochtml.log when an error (warning) occurs?

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

d70709f	Merge branch 'develop' of git://github.com/sagemath/sage into public/documentation/add_traceback
f10a30c	Don't try to prickle the complete traceback

[sagetrac-git]

Changed commit from 612e0c1 to f10a30c

[tobiasdiez]

comment:5

Thanks for the review. I've now extended the ticket description and fixed the doctests.

[mkoeppe]

Reviewer: https://github.com/mkoeppe/sage/actions/runs/524223614, https://github.com/mkoeppe/sage/actions/runs/524223611

[mkoeppe]

comment:6

Let's see if this can help shed some light on previously reported docbuild failures such as #30640, #30796, #30825

[jhpalmieri]

comment:7

I've been having problems building the documentation on OS X Big Sur with the latest Xcode + homebrew. This ticket didn't have any apparent effect on the traceback.

[tobiasdiez]

comment:8

@jhpalmieri: So you mean you got an exception in one of the workers, but the traceback still didn't included the proper origin of the exception? That's a bit strange, it's working for me.

[jhpalmieri]

comment:9

Yes, with the failure in #31344.

[tobiasdiez]

comment:10

That makes sense. In #31344, no exception is thrown but the process ends with an error code, which then triggers

        if exitcode != 0:
            raise WorkerDiedException(f"worker for {task[1]} died with non-zero exit code {w.exitcode}")

In this case, no stacktrac is added since this information is simply not available. The worker needs to throw an exception (including a stacktrace) so that the exception handler in

         try:
             result = target(task)
         except BaseException as exc:
             queue.put((None, exc))
             queue.put((None, RemoteExceptionWrapper(exc)))

is invoked. This needs changes to the particular worker, and should be done in a follow-up issue.

[mkoeppe]

comment:11

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review of ticket status, priority, and last modification date.

[mkoeppe]

comment:12

Setting a new milestone for this ticket based on a cursory review.

[sagetrac-git]

Changed commit from f10a30c to d93695d

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

d93695d

Merge branch 'develop' of https://github.com/sagemath/sage into public/documentation/add_traceback

[mkoeppe]

Changed reviewer from https://github.com/mkoeppe/sage/actions/runs/524223614, https://github.com/mkoeppe/sage/actions/runs/524223611 to none

[orlitzky]

comment:15

Did the GH actions pass? My only suggestion would be to make _rebuild_exc a class method instead of a top-level function, but it's not important. This is a great improvement. Trying to track down all of the docbuild failures with --underscore and --include-tests-blocks is always a nightmare.

[sagetrac-git]

Changed commit from d93695d to 37f1d0c

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

37f1d0c

Move _rebuild_exc and cleanup code

[tobiasdiez]

comment:17

Thanks for the review. I've now moved _rebuild_exc to a local method in reduce.
Not sure if the tests passed but patch bot seems to be green.

[orlitzky]

comment:18

Replying to @tobiasdiez:

Thanks for the review. I've now moved _rebuild_exc to a local method in reduce.
Not sure if the tests passed but patch bot seems to be green.

Hm, this is even better. But now another refactoring is suggesting itself. Instead of,

def __reduce__(self):
    def _rebuild_exc(exc: BaseException, tb: str):
        """
        Reconstructs the exception, putting the original exception as cause.
        """
        exc.__cause__ = RemoteException(tb)
        return exc

    return _rebuild_exc, (self.exc, self.tb)

couldn't we just do

def __reduce__(self):
    self.exc.__cause__ =  RemoteException(self.tb)
    return self.exc

?

I also think we'll need doctests for the new classes/methods even if they're pretty trivial. Otherwise our coverage goes down.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

45ef9e3

Fix code and add doctest

[sagetrac-git]

Changed commit from 37f1d0c to 45ef9e3

[tobiasdiez]

comment:20

Replying to @orlitzky:

def __reduce__(self):
    self.exc.__cause__ =  RemoteException(self.tb)
    return self.exc

I don't have much experience with pickling, but https://docs.python.org/3/library/pickle.html#object.__reduce__ says that either a string or tuple should be returned.

My solution also didn't worked as __reduce__ cannot return locally defined methods. So I've converted it to a static instance method as you initially suggested.

I also think we'll need doctests for the new classes/methods even if they're pretty trivial. Otherwise our coverage goes down.

I've added a doctest for the reduce method. The other methods are still covered by the existing doctests.

[orlitzky]

comment:21

Replying to @tobiasdiez:

I also think we'll need doctests for the new classes/methods even if they're pretty trivial. Otherwise our coverage goes down.

I've added a doctest for the reduce method. The other methods are still covered by the existing doctests.

I know, I was referring specifically to (before):

$ sage -coverage src/sage_docbuild/utils.py 
------------------------------------------------------------------------
SCORE src/sage_docbuild/utils.py: 50.0% (1 of 2)

Missing documentation:
     * line 10: def __init__(self, message, original_exception=None)
------------------------------------------------------------------------

and (after):

$ sage -coverage src/sage_docbuild/utils.py 
------------------------------------------------------------------------
SCORE src/sage_docbuild/utils.py: 28.6% (2 of 7)

Missing documentation:
     * line 16: def __init__(self, tb:str)
     * line 19: def __str__(self)
     * line 32: def __init__(self, exc:BaseException)
     * line 65: def __init__(self, message:Optional[str], original_exception:Optional[BaseException]=None)

Missing doctests:
     * line 42: def _rebuild_exc(exc:BaseException, tb:str)
------------------------------------------------------------------------

Normally the patchbots would have complained (see the "coverage" plugin), but in this case I think the special nature of the sage_docbuild package avoided it.

I've been on the other side of this argument more than once, so I feel for you, but the developer's guide still states that every function needs a docstring: https://doc.sagemath.org/html/en/developer/coding_basics.html#documentation-strings

[sagetrac-git]

Changed commit from 45ef9e3 to c37d055

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

756a79d	Merge branch 'develop' of https://github.com/sagemath/sage into public/documentation/add_traceback
c37d055	Add docstrings

[tobiasdiez]

comment:23

I've added the missing docstrings. But since none of the methods in this class have any tests, I don't have that much motivation to add tests for trivial constructor / properties ;-)

[tobiasdiez]

comment:24

Test bot is green, anything else to do?

[orlitzky]

comment:25

I forgot what I was waiting for, so it must be OK.

[orlitzky]

Reviewer: Michael Orlitzky

[tobiasdiez]

comment:26

Thanks!

[vbraun]

Changed branch from public/documentation/add_traceback to c37d055