Make pylab import all configurable #622

jenshnielsen · 2011-07-24T10:57:57Z

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 · 2011-08-18T06:59:32Z

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 · 2011-08-18T08:05:25Z

@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 · 2011-08-19T04:46:02Z

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 · 2011-08-21T13:17:56Z

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 · 2011-08-22T01:18:02Z

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 · 2011-09-03T11:02:21Z

IPython/core/magic.py

-        self.shell.enable_pylab(s)
+
+        try:
+            ip = get_ipython()


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

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?.

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?

current Application can always be retrieved with:

from IPython.config.application import Application app = Application.instance()

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.

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.

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.

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.

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 · 2011-09-08T18:54:08Z

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 · 2011-09-10T04:24:27Z

IPython/lib/tests/test_irunner_pylab_magic.py

+    @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)


Is this line supposed to do anything?

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

Thanks. I have removed the useless line.

minrk · 2011-09-10T04:50:36Z

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 · 2011-09-11T11:50:26Z

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

minrk · 2011-09-12T04:15:14Z

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 · 2011-09-12T06:24:55Z

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.

…cation if initialized.

jenshnielsen · 2011-09-12T16:57:18Z

Rebased

minrk · 2011-09-12T17:33:42Z

Thanks! Looks good to me, passes tests.

Merged.

takluyver reviewed Sep 3, 2011
View reviewed changes

minrk reviewed Sep 10, 2011
View reviewed changes

jenshnielsen added 2 commits September 11, 2011 18:45

Make import_all configurable in InteractiveShellApp

84cb7c3

Add skipif_not_matplotlib decorator

cd5f956

jenshnielsen added 2 commits September 12, 2011 18:48

Make pylab magic respect the pylab_import_all config. Read from appli…

a3555de

…cation if initialized.

Add irunner test for pylab_import_all.

0fc7291

minrk merged commit 0fc7291 into ipython:master Sep 12, 2011

jenshnielsen mentioned this pull request Sep 13, 2011

Using irunner from virtual evn uses systemwide ipython #779

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make pylab import all configurable #622

Make pylab import all configurable #622

jenshnielsen commented Jul 24, 2011

fperez commented Aug 18, 2011

minrk commented Aug 18, 2011

fperez commented Aug 19, 2011

jenshnielsen commented Aug 21, 2011

fperez commented Aug 22, 2011

takluyver Sep 3, 2011

jenshnielsen Sep 4, 2011

takluyver Sep 4, 2011

minrk Sep 5, 2011

jenshnielsen Sep 6, 2011

takluyver Sep 6, 2011

minrk Sep 7, 2011

jenshnielsen Sep 7, 2011

minrk Sep 7, 2011

jenshnielsen commented Sep 8, 2011

minrk Sep 10, 2011

minrk Sep 10, 2011

jenshnielsen Sep 11, 2011

minrk commented Sep 10, 2011

jenshnielsen commented Sep 11, 2011

minrk commented Sep 12, 2011

jenshnielsen commented Sep 12, 2011

jenshnielsen commented Sep 12, 2011

minrk commented Sep 12, 2011

Make pylab import all configurable #622

Make pylab import all configurable #622

Conversation

jenshnielsen commented Jul 24, 2011

fperez commented Aug 18, 2011

minrk commented Aug 18, 2011

fperez commented Aug 19, 2011

jenshnielsen commented Aug 21, 2011

fperez commented Aug 22, 2011

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenshnielsen commented Sep 8, 2011

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minrk commented Sep 10, 2011

jenshnielsen commented Sep 11, 2011

minrk commented Sep 12, 2011

jenshnielsen commented Sep 12, 2011

jenshnielsen commented Sep 12, 2011

minrk commented Sep 12, 2011