Make pylab import all configurable #622

Merged
merged 4 commits into from Sep 12, 2011

Projects

None yet

4 participants

@jenshnielsen
Contributor

New version of pull request #551 This should of cause not be merged until after 0.11
I have added a test and changed the magic function to use the import_all setting.

I few issues remain:

I don't like the way that the magic function works as the default value True is hard coded when get_ipython().TerminalIPythonApp.pylab_import_all is not set. Is there a simple way to get the default value here?

The test uses the magic function. It results in 2 warnings because of problems with the gtk hook and because matplotlib.use. It would be better with a test that creates a new instance of IPython only for the test. I tried creating a new instance with code similar to the TerminalIPythonApp launch_new_instance. However this seems to lock the test framework. Any hints for making a better test are welcome.

@fperez
Member
fperez commented Aug 18, 2011

This looks good, but I have a couple of comments:

  • Your test code can be simplified by replacing
        ip.magic('pylab')
        ip.run_cell('a = dir()')
        ipdir = ip.user_ns['a']
        has_plot = False
        for ipd in ipdir:
            if ipd == 'plot':
                has_plot = True
        self.assertTrue(has_plot)
    

with

        ip.magic('pylab')
        self.assertTrue('plot' in ip.user_ns)

which is clearer, faster and fully equivalent.

  • The flag should really be added to core/shellapp.py, where incidentally all the pylab flags should be moved (right now we have duplicated them in both the terminal and the qt console). By doing this, we'll remove the duplication and your new option will work for both the terminal and the qt console, which right now it doesn't.
  • @minrk, could you help me out with the following: which is the object that has the fully merged configuration data, that includes the overlaying of the various defaults with the stuff in get_ipython().config? I'm still a bit fuzzy on those details.

In summary, I think if we work a bit on this one, not only will we get the import_all situation fixed, but more generally where we handle pylab for all ipython shells.

@minrk
Member
minrk commented Aug 18, 2011
  • @minrk, could you help me out with the following: which is the object that has the fully merged configuration data, that includes the overlaying of the various defaults with the stuff in get_ipython().config? I'm still a bit fuzzy on those details.

There is only ever one config object in the entire application, and get_ipython().config will return it.

There is no store of defaults other than the classes themselves, so the only 'merged' value is in the actual traits of configured instances.

@fperez
Member
fperez commented Aug 19, 2011

Got it, thanks @minrk. So indeed, @jenshnielsen, the way to go is to clean things out and put the pylab flags only in the core.shellapp.InteractiveShellApp object, and then both the terminal and the Qt console will get them.

From looking around, I get the feeling that we need to do a little cleanup of all the classes, but you don't need to worry about that quite now.

@jenshnielsen
Contributor

I changed the test and moved pylab_import_all to core.shellapp.InteractiveShellApp
However I did not move the pylab option. The problem is that it that the pylab flag has a inline option
in the qtconsole. The terminal app does of cause not have this option.

The test is still potentially problematic because pylab is activated in the testing env. and I guess that this
can give problems in the following tests. Maybe one could implement a none option for pylab that only does the
import but doesn't activate the gui integration.

@fperez
Member
fperez commented Aug 22, 2011

Thanks much. I won't have time to finish this in the next couple of days, but hopefully on Wednesday at the mini-sprint prior to euroscipy I can finish it up.

@takluyver takluyver and 2 others commented on an outdated diff Sep 3, 2011
IPython/core/magic.py
@@ -3454,7 +3454,14 @@ Defaulting color scheme to 'NoColor'"""
Backend in use: Qt4Agg
For more information, type 'help(pylab)'.
"""
- self.shell.enable_pylab(s)
+
+ try:
+ ip = get_ipython()
@takluyver
takluyver Sep 3, 2011 Member

I think this should be the same object as self.shell in this context, so you needn't use get_ipython.

@jenshnielsen
jenshnielsen Sep 4, 2011 Contributor

Yes thats simpler, thanks. The try except AttributeError: construction
still seems to be necessary as pylab_import_all has doesn't have any
value if it hasn't been customized in a config file. Is there a simple way to get the default value
so that true doesn't have to be hard coded here?.

@takluyver
takluyver Sep 4, 2011 Member

Hmm. I think you're currently using a reference to the config itself. What we'd need is a reference to the application object itself, but I don't know if it's possible to get one from the shell object.

@fperez, @minrk : Is there a way to get a reference to the application from inside a magic method?

@minrk
minrk Sep 5, 2011 Member

current Application can always be retrieved with:

from IPython.config.application import Application
app = Application.instance()
@jenshnielsen
jenshnielsen Sep 6, 2011 Contributor

That works except in the test. The test returns an instance of the basic application without pylab_import_all.
In the IPython shell it returns a TerminalIPythonApp instance that has pylab_import_all
defined. I worked around this by falling back to self.shell.config.TerminalIPythonApp.pylab_import_all
that works in the test. But is not defined in the IPython shell when it hasn't been customized.

@takluyver
takluyver Sep 6, 2011 Member

Generally I prefer to avoid having code in the core just for compatibility with the test suite. Two possible ways round it:

  • The test calls Application.instance() before the code, and sets a pylab_import_all attribute.
  • The test runs a separate IPython process and gets it to print some output indicating whether or not names from pylab have been imported.

Thanks for this work - I hope it doesn't feel like a wild goose chase.

@minrk
minrk Sep 7, 2011 Member

Yes, thanks for working on this. The configuration stuff is definitely going through some growing pains, as it's brand new and not the most convenient for everything. The test suite should presumably start an Application, so its environment is more like that seen by a real IPython, but that's out of scope for this.

For now, I would create the Application in your tests, rather than writing to the config object itself. Add:

app = Application.instance()
app.pylab_import_all = True/False

in your tests,

and then check in the magic should not go directly to the config object, but straight to a default in the application doesn't have the right attribute:

        if Application.initialized():
            app = Application.instance()
            try:
                import_all_status = app.pylab_import_all
            except AttributeError:
                import_all_status = True # (or False, whatever the default is)
        else:
            import_all_status = True

That way you can have a test that includes the case where the Application does not even have the trait.

@jenshnielsen
jenshnielsen Sep 7, 2011 Contributor

Thanks I have implemented this exactly as you suggested and it works.
I am still a bit worried about the test. I think we need a way to avoid that the pylab magic activates the gui
in order to avoid problems in subsequent tests.

@minrk
minrk Sep 7, 2011 Member

If you are concerned about polluting the environment, @takluyver is right, performing the test via an IPython subprocess is safer. There are various irunner tests already in the test suite that give examples of this.

@jenshnielsen
Contributor

New version of the test using now irunner. I copied and modified the test_irunner.py a bit.
It was needed to change it to use a regex because pylab prints text that depend on the backend.

Note that one of the tests fails when running it from virtualenv. The virualenv irunner seems to use the system wide
ipython install not the one in the virtualenv. (Verified by inspecting get_ipython parameters from the virualenv)

@minrk minrk and 1 other commented on an outdated diff Sep 10, 2011
IPython/lib/tests/test_irunner_pylab_magic.py
+In \[2\]: app = Application\.instance\(\)
+In \[3\]: app\.pylab_import_all = True
+In \[4\]: pylab
+^Welcome to pylab, a matplotlib-based Python environment
+For more information, type 'help\(pylab\)'\.
+In \[5\]: ip=get_ipython\(\)
+In \[6\]: \'plot\' in ip\.user_ns
+Out\[6\]: True
+"""
+ runner = irunner.IPythonRunner(out=self.out)
+ self._test_runner(runner,source,output)
+
+ @decorators.skipif_not_matplotlib
+ def test_pylab_import_all_disabled(self):
+ "Verify that plot is not available when pylab_import_all = False"
+ runner = irunner.PythonRunner(out=self.out)
@minrk
minrk Sep 10, 2011 Member

Is this line supposed to do anything?

@minrk
minrk Sep 10, 2011 Member

There's a duplicate line in the other test. Presumably leftovers from the copy/paste.

@jenshnielsen
jenshnielsen Sep 11, 2011 Contributor

Thanks. I have removed the useless line.

@minrk
Member
minrk commented Sep 10, 2011

The real code looks great to me.

I'm not sure why the irunner would not spawn the same IPython it's being run from - it does inherit the path/env as I understand, but virtualenv can fail at such things. It is possible that the path/env does not get set up or inherited properly, but this has not been my experience.

@jenshnielsen
Contributor

I submitted a new issue #779 for the irunner problem. It is independent of both this pull request and the general testing
env.

@minrk
Member
minrk commented Sep 12, 2011

Okay, I think this looks good. Thanks for opening the virtualenv-test Issue. It now conflicts with a big recent py3compat merge, so if you rebase again on master, I will do the merge. If you'd rather not do that, I can to a rebased merge myself.

@jenshnielsen
Contributor

Thanks. I have already another local branch with the changes squashed into 4 logical commits and
rebased on master from yesterday. I will push that on top of this one later today.

@jenshnielsen
Contributor

Rebased

@minrk
Member
minrk commented Sep 12, 2011

Thanks! Looks good to me, passes tests.

Merged.

@minrk minrk merged commit 0fc7291 into ipython:master Sep 12, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment