Skip to content

Sphinxext: display shorter error traceback when :okexcept: is specified #8686

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

jorisvandenbossche
Copy link
Contributor

This is a small patch to push a pandas change upstream (pandas-dev/pandas#10727).

When using :okexcept: for code blocks that are not suppressed (e.g. you want to show that a certain line of code raises an error), at this moment the full error traceback is shown. While for documentation/education purposes, it is enough/better to only show the error message itself.

E.g instead of the full traceback you see in the docs here: http://pandas.pydata.org/pandas-docs/version/0.16.2/timeseries.html#ambiguous-times-when-localizing, you would get:

In [292]: rng_hourly.tz_localize('US/Eastern')
AmbiguousTimeError: Cannot infer dst time from Timestamp('2011-11-06 01:00:00'), try using the 'ambiguous' argument

Open questions:

  • I now just changed the behaviour, but this could also be controlled by a new option or a new pseudo-decorator if needed.
  • I now only show the error message itself, but it is also possible to e.g. show the starting line (------...) of the traceback as well.

cc @chebee7i @tacaswell (as other users of / contributors to the ipython directive)

@takluyver
Copy link
Member

Not entirely sure about this - wouldn't you want the output to match what a user would see if they actually run that?

On the other hand, since we don't use the IPython Sphinx directive ourselves, I'm happy for those who do to take the lead.

@jorisvandenbossche
Copy link
Contributor Author

I understand, and that's why I certainly can make it optional if needed.

For our docs, I think that the full traceback only adds clutter to the documentation, that can overshadow what you actually want to show (error tracebacks can get much longer than the one in the example I gave). Furthermore, the full traceback is most of the time not that interesting (apart from where it was initially raised, but for the docs this is something like <ipython-input-292-8c5fa6a37f5b> in <module>() ----> 1 rng_hourly.tz_localize('US/Eastern')) and contains some info that is specific to the user anyway (the paths of the files).

@takluyver
Copy link
Member

That makes sense. I think I'd lean towards making an option for it, but it's probably best to ask the users: would anyone enable 'show the full traceback' if it was an option?

@tacaswell
Copy link
Contributor

I would like a truncated traceback that shows the inner most frame or two. Gives a bit more context to match what the users see when something blows up.

@jorisvandenbossche
Copy link
Contributor Author

@tacaswell What do you mean exactly with the inner most?

Using the example from the docs I linked, I would expect that you mean:

In [292]: rng_hourly.tz_localize('US/Eastern')

/home/joris/scipy/pandas/pandas/tseries/index.pyc in tz_localize(self, tz, ambiguous)
   1619 
   1620             new_dates = tslib.tz_localize_to_utc(self.asi8, tz,
-> 1621                                                  ambiguous=ambiguous)
   1622         new_dates = new_dates.view(_NS_DTYPE)
   1623         return self._shallow_copy(new_dates, tz=tz)

/home/joris/scipy/pandas/pandas/tslib.so in pandas.tslib.tz_localize_to_utc (pandas/tslib.c:47148)()

AmbiguousTimeError: Cannot infer dst time from Timestamp('2011-11-06 01:00:00'), try using the 'ambiguous' argument

while I would think that for a user the outer most frame (where it is raised in the user his code) is the most useful to see:

In [292]: rng_hourly.tz_localize('US/Eastern')
---------------------------------------------------------------------------
AmbiguousTimeError                        Traceback (most recent call last)
<ipython-input-292-8c5fa6a37f5b> in <module>()
----> 1 rng_hourly.tz_localize('US/Eastern')

AmbiguousTimeError: Cannot infer dst time from Timestamp('2011-11-06 01:00:00'), try using the 'ambiguous' argument

But of course, for the docs it is maybe not really about what is useful for the user in real world, but that it matches somewhat the actual output.

To meet the different needs, I could eg make a config option show_error_traceback_frames which is by default None (show all), but you can set this to a number (0 -> show no frames apart from the actual message, 1 -> show the 1 inner most, ...). That should be rather easy (the frames are delineated by blank lines)

@tacaswell
Copy link
Contributor

My thought was that if the user is sitting in the repl and then get a deep trackback they will only be able to see the inner frame or two so showing them that will help with scanning/pattern matching.

# if :okexcept: has been specified, display shorter traceback
if is_okexcept and "Traceback" in processed_output:
traceback = processed_output.split('\n')
processed_output= '\n'.join(traceback[-2:])
Copy link
Member

Choose a reason for hiding this comment

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

processed_output is all the output corresponding to that chunk of code, right? So if anything is printed before the traceback, this discards it?

@Carreau
Copy link
Member

Carreau commented Feb 22, 2016

What the status of this ?

@jorisvandenbossche
Copy link
Contributor Author

The status is that I have to look at this again, but do not have the time right now (trying to finish a phd and such things ...).
I still think this is potentially useful, but the current implementation should be better tested (the change I did in the pandas version of the ipython directive was reverted because it gave some errors on travis for some reason I was not yet able to understand, locally it runs fine)

@jorisvandenbossche
Copy link
Contributor Author

So you can close it with a closed-pr label for now, unless someone wants to take this up in the short term

@Carreau
Copy link
Member

Carreau commented Feb 22, 2016

No problem. Get your PhD. I'll see if we can find someone to revive this !

@Carreau
Copy link
Member

Carreau commented Apr 7, 2016

Tagging as closed-PR, and closing as this is seem to have stalled a bit.
This is just to keep the PR queue a bit shorter and focusing on what is actually worked on.
Feel free to reopen, (or ask for reopening if you don't have enough right to do so).
If you were not the original author, and want to revive this, please feel free to do so.

Also, we are doing our best to enable people to learn how to contribute to the project, and get commits in. If you are missing time or have any other issues that prevent you to work on that, please feel free to tell us, we can try to build on top of what you did.

Thanks a lot and looking forward to see this moving forward again.

@jorisvandenbossche : See you after your PhD ! Hope things are moving forward !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants