Opened 7 years ago

Closed 7 years ago

#14188 closed enhancement (fixed)

IPython 0.13: merge user configuration with Sage configuration

Reported by: jhpalmieri Owned by: jason
Priority: minor Milestone: sage-5.8
Component: misc Keywords: IPython config
Cc: vbraun, jason Merged in: sage-5.8.beta3
Authors: John Palmieri, Volker Braun Reviewers: Volker Braun, William Stein, John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

After #12719, user configuration files (in .sage/ipython-0.12/profile_default/ and .../profile_sage/) seem to be effectively ignored: I think they are read after the Sage terminal app has already been initialized, at which point it is too late.

The attached patch remedies this, reading the configuration from .sage/ipython-0.12/profile_default/ipython_config.py during the initialization. Question: should it read from this directory or from profile_sage?

Apply trac_14188_vb.patch and trac_14188_docfix.patch

Attachments (2)

trac_14188_vb.patch (3.6 KB) - added by vbraun 7 years ago.
Reworked patch
trac_14188_docfix.patch (1.4 KB) - added by vbraun 7 years ago.
Improved patch

Download all attachments as: .zip

Change History (25)

comment:1 Changed 7 years ago by vbraun

It should read from /profile_sage/ipython_config.py for sure.

comment:2 Changed 7 years ago by jhpalmieri

  • Status changed from new to needs_review

comment:3 Changed 7 years ago by jhpalmieri

Here's a new version which reads from profile_sage.

Changed 7 years ago by vbraun

Reworked patch

comment:4 Changed 7 years ago by vbraun

I changed your code to override load_config_file() instead of stuffing it in the ctor. Now the configuration is only loaded once, as one can easily check by putting a print statement into ipython_config.py.

Also, after discussion with Jason and William we decided to default to colors in the terminal. For simplicity I just added this into this patch.

comment:5 Changed 7 years ago by vbraun

  • Authors changed from John Palmieri to John Palmieri, Volker Braun
  • Description modified (diff)
  • Reviewers set to Volker Braun, William Stein

And I agree with your changes, of course. Feel free to set it to positive review if you are happy with it.

comment:6 Changed 7 years ago by jhpalmieri

  • Reviewers changed from Volker Braun, William Stein to Volker Braun, William Stein, John Palmieri
  • Status changed from needs_review to positive_review

comment:7 Changed 7 years ago by klee

Apply trac_14188_vb.patch

comment:8 Changed 7 years ago by vbraun

  • Description modified (diff)

And IPython is of course not smart enough to turn off colors when the output is a pipe, thanks a bunch. The second patch is the one-line fix to do it by hand.

comment:9 Changed 7 years ago by jdemeyer

  • Status changed from positive_review to needs_work

comment:10 Changed 7 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Second patch needs review.

comment:11 Changed 7 years ago by jhpalmieri

If someone customizes their IPython configuration, this can cause doctests to fail (for example, if you turn on an output prompt like "Out[1]:", interfaces/sage0.py fails). I think this is okay, but I'm going to post at #12415 about it.

comment:12 Changed 7 years ago by jhpalmieri

  • Status changed from needs_review to positive_review

All tests passed.

comment:13 Changed 7 years ago by jdemeyer

  • Status changed from positive_review to needs_work

comment:15 Changed 7 years ago by jhpalmieri

I see this on bsd.math.washington.edu (OS X 10.6), but I have no problems on lena.

comment:16 follow-up: Changed 7 years ago by jhpalmieri

A typical failure on OS X 10.6:

    sage: subsage = Sage()
    sage: s = ZZ(subsage('initial_seed()'))
    <boom>

It looks like when running Sage as a subprocess on this platform, sys.stdout.isatty() returns True, even while doctesting. How else can we detect this situation?

comment:17 Changed 7 years ago by vbraun

Pexpect runs in a pty, so on any platform isatty() will return true. This is why we explicitly disable colors when starting a sage-in-sage session. My current theory is that the pty gets messed up somehow by the ansi codes since we disable colors by sending %colors NoColor, so at least one colored prompt will be displayed. I'm currently trying if disabling color via command line switch works...

comment:18 in reply to: ↑ 16 Changed 7 years ago by jdemeyer

Replying to jhpalmieri:

How else can we detect this situation?

pexpect children have the environment variable TERM unset (#12221, similar: #12263). We did this precisely to solve problems like this where the terminal did strange things.

sage: "TERM" in os.environ
True
sage: sage0('"TERM" in os.environ')
False

comment:19 Changed 7 years ago by jhpalmieri

A change like this fixes the failures on bsd.math.washington.edu. I haven't run a full test suite yet.

  • sage/misc/interpreter.py

    diff --git a/sage/misc/interpreter.py b/sage/misc/interpreter.py
    a b  
    660660        verbose_crash = True),
    661661    TerminalInteractiveShell = Config(
    662662        ast_node_interactivity = 'all',
    663         colors = 'LightBG' if sys.stdout.isatty() else 'NoColor',
     663        colors = 'LightBG' if (sys.stdout.isatty() and 'TERM' in os.environ) else 'NoColor',
    664664        confirm_exit = False,
    665665        separate_in = ''),
    666666    # The extension is *always* loaded for SageTerminalApp

Changed 7 years ago by vbraun

Improved patch

comment:20 Changed 7 years ago by vbraun

I'd rather always turn off colors in the sage<->sage interface than rely on the TERM variable. The updated patch fixes sage0 on OSX (bsd.math) for me.

comment:21 Changed 7 years ago by vbraun

  • Status changed from needs_work to needs_review

comment:22 Changed 7 years ago by jhpalmieri

  • Status changed from needs_review to positive_review

This passes tests for me on bsd.

comment:23 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.8.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.