sagemath/sage

ascii_art fail in jupyter notebook

Closed this issue · 19 comments

When running sage in Jupyter notebook,

sage: ascii_art(list(Partitions(5)))
<repr(<sage.typeset.ascii_art.AsciiArt at 0x7f8f358aa250>) failed: OSError: [Errno 25] Inappropriate ioctl for device>

This is because the _terminal_width() method does not work with Jupiter.

Component: user interface

Author: Kwankyu Lee

Branch/Commit: 5aa8cd5

Reviewer: Dima Pasechnik

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

New commits:

ed22f90Replace fileno() hack with isatty()

Commit: ed22f90

Author: Kwankyu Lee

comment:4

This removed try/except block. It's not clear why it was there in the 1st place, but would it be easier to keep it?

comment:5

Replying to @dimpase:

This removed try/except block. It's not clear why it was there in the 1st place, but would it be easier to keep it?

It is explained in the comment. Not necessary anymore.

comment:6

There could still be cases we don't know about, e.g. emacs mode, the modes used by one of these VSCode plugins, etc...

comment:7

Replying to @dimpase:

There could still be cases we don't know about, e.g. emacs mode, the modes used by one of these VSCode plugins, etc...

According to the comment, the author of the code, Volker, only considered terminal and IPython. So the try/except code is only for those two cases.

There is no reason to suspect that this change will make the situation with other user interfaces worse or better... In any case, it could be dealt with in other tickets if reported by users of those interfaces.

comment:8

Replying to @kwankyu:

In any case, it could be dealt with in other tickets if reported by users of those interfaces.

Or in this ticket. We can wait.

comment:9

Replying to @kwankyu:

There is no reason to suspect that this change will make the situation with other user interfaces worse or better...

If any other user interface is affected, then it would be rather because of replacing os.isatty(sys.stdout.fileno()) with sys.stdout.isatty().

comment:10

I put a post in sage-devel: https://groups.google.com/g/sage-devel/c/EHCPeH5BXh4

comment:11

to copy my sage-dev message:


I think this fix is an API change. We have no control over what fake
ttys Sage users are using.
What earlier returned False might now lead to an error.
So if you want to remove that try/except, you must deprecate it first.

IMHO I'd better have try/except retained (your fix applied in the body of try)

comment:12

By the way, I checked that emacs-mode works with this change. Another tricky case might be sage cell.

Changed commit from ed22f90 to 5aa8cd5

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

5aa8cd5Retain try/except clause
comment:14

Replying to @dimpase:

IMHO I'd better have try/except retained (your fix applied in the body of try)

Okay. Being cautious doesn't hurt. Thanks.

comment:15

OK, good.

Reviewer: Dima Pasechnik

Changed branch from u/klee/33996 to 5aa8cd5