Skip to content
This repository

Switch to simple `__IPYTHON__` global #1059

Merged
merged 3 commits into from over 2 years ago

2 participants

Fernando Perez Min RK
Fernando Perez
Owner

In 0.11 we added __IPYTHON__active as a global, managed with the builtin trap, to try and detect nested ipythons. But in reality that mechanism is very fragile, the values aren't always right, and I think it's just unnecessary complexity. People do ask often, however, for some way of knowing whether they're running in ipython or not.

So this pr proposes that we simply create a global flag in __builtin__, called __IPYTHON__ (like the name we used to have and that it turns out people were already relying on). This flag will just be set to True at shell creation, with no attempt to delete it, or otherwise manage 'activity' (in event driven contexts this is a futile battle). I think this is the simplest thing we can do that will reasonable cover most cases without making promises we can't keep.

I'd like this merged before 0.12 so we're back to an api similar to what we had for most of our life from now on. I have the feeling that 0.12 will be when many people start really porting their codes forward, so minimizing api breakage here is worthwhile.

Min RK minrk commented on the diff November 28, 2011
IPython/core/interactiveshell.py
@@ -573,6 +573,20 @@ class InteractiveShell(SingletonConfigurable, Magic):
573 573
             self.magic_logstart()
574 574
 
575 575
     def init_builtins(self):
  576
+        # A single, static flag that we set to True.  Its presence indicates
  577
+        # that an IPython shell has been created, and we make no attempts at
  578
+        # removing on exit or representing the existence of more than one
  579
+        # IPython at a time.
  580
+        builtin_mod.__dict__['__IPYTHON__'] = True
  581
+
  582
+        # In 0.11 we introduced '__IPYTHON__active' as an integer we'd try to
  583
+        # manage on enter/exit, but with all our shells it's virtually
  584
+        # impossible to get all the cases right.  We're leaving the name in for
  585
+        # those who adapted their codes to check for this flag, but will
  586
+        # eventually remove it after a few more releases.
  587
+        builtin_mod.__dict__['__IPYTHON__active'] = \
  588
+                                          'Deprecated, check for __IPYTHON__'
3
Min RK Owner
minrk added a note November 28, 2011

I don't know if anyone was actually doing this, but if anyone was using this as the counter that it is, this will still break it. Do we want to worry about that?

Fernando Perez Owner
fperez added a note November 29, 2011

Yes, I realize that. But I really doubt anyone was, given that even we were inconsistent in using it (the value isn't the same in kernels than it is in the terminal b/c our update logic was in the wrong place). So I think this is just an example of an api that didn't really work out, and which we might as well kill quickly rather than let it live further and possibly get entrenched when it shouldn't.

Min RK Owner
minrk added a note November 29, 2011

Makes perfect sense, I just wanted to check (and have public record of the decision).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Min RK
Owner

This seems quite sensible, and seems ready for merge.

What are the official ways to:

a) determine that you are in an IPython interactive namespace (e.g. %run -i)
b) determine that you are in a process with IPython instance somewhere.

I've been checking for the existence of get_ipython for b), but I don't know about a). If I read correctly, __IPYTHON__ is also for b).

Fernando Perez
Owner

Yes, I guess part of the intent of this PR is for us to rationalize this... I'd say that __IPYTHON__ is for b), and I guess we don't really have anything special for a), because get_ipython gets injected into enough places that even a plain %run (without -i) will see it. Perhaps we should add another flag, say __IPYTHON__interactive for this, that would only be written to the user_ns when we prepare it? Kind of a mouthful, but if that sound like a good idea we can mull on the name.

Min RK
Owner

Since we have never had an answer for a), maybe we shouldn't be unnecessarily intrusive in the user_ns. If someone asks for it, then we can.

Fernando Perez
Owner

Great, that's a good plan. We'll then merge this only with __IPYTHON__, which should be sufficient and does simplify things out. Thanks for the review.

Fernando Perez fperez merged commit 487b6b9 into from December 05, 2011
Fernando Perez fperez closed this December 05, 2011
Fernando Perez fperez referenced this pull request from a commit January 10, 2012
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
28  IPython/core/builtin_trap.py
@@ -88,17 +88,12 @@ def add_builtin(self, key, value):
88 88
             self._orig_builtins[key] = orig
89 89
             bdict[key] = value
90 90
 
91  
-    def remove_builtin(self, key):
  91
+    def remove_builtin(self, key, orig):
92 92
         """Remove an added builtin and re-set the original."""
93  
-        try:
94  
-            orig = self._orig_builtins.pop(key)
95  
-        except KeyError:
96  
-            pass
  93
+        if orig is BuiltinUndefined:
  94
+            del __builtin__.__dict__[key]
97 95
         else:
98  
-            if orig is BuiltinUndefined:
99  
-                del __builtin__.__dict__[key]
100  
-            else:
101  
-                __builtin__.__dict__[key] = orig
  96
+            __builtin__.__dict__[key] = orig
102 97
 
103 98
     def activate(self):
104 99
         """Store ipython references in the __builtin__ namespace."""
@@ -107,22 +102,11 @@ def activate(self):
107 102
         for name, func in self.auto_builtins.iteritems():
108 103
             add_builtin(name, func)
109 104
 
110  
-        # Keep in the builtins a flag for when IPython is active.  We set it
111  
-        # with setdefault so that multiple nested IPythons don't clobber one
112  
-        # another.
113  
-        __builtin__.__dict__.setdefault('__IPYTHON__active', 0)
114  
-
115 105
     def deactivate(self):
116 106
         """Remove any builtins which might have been added by add_builtins, or
117 107
         restore overwritten ones to their previous values."""
118  
-        # Note: must iterate over a static keys() list because we'll be
119  
-        # mutating the dict itself
120 108
         remove_builtin = self.remove_builtin
121  
-        for key in self._orig_builtins.keys():
122  
-            remove_builtin(key)
  109
+        for key, val in self._orig_builtins.iteritems():
  110
+            remove_builtin(key, val)
123 111
         self._orig_builtins.clear()
124 112
         self._builtins_added = False
125  
-        try:
126  
-            del __builtin__.__dict__['__IPYTHON__active']
127  
-        except KeyError:
128  
-            pass
14  IPython/core/interactiveshell.py
@@ -573,6 +573,20 @@ def init_logstart(self):
573 573
             self.magic_logstart()
574 574
 
575 575
     def init_builtins(self):
  576
+        # A single, static flag that we set to True.  Its presence indicates
  577
+        # that an IPython shell has been created, and we make no attempts at
  578
+        # removing on exit or representing the existence of more than one
  579
+        # IPython at a time.
  580
+        builtin_mod.__dict__['__IPYTHON__'] = True
  581
+
  582
+        # In 0.11 we introduced '__IPYTHON__active' as an integer we'd try to
  583
+        # manage on enter/exit, but with all our shells it's virtually
  584
+        # impossible to get all the cases right.  We're leaving the name in for
  585
+        # those who adapted their codes to check for this flag, but will
  586
+        # eventually remove it after a few more releases.
  587
+        builtin_mod.__dict__['__IPYTHON__active'] = \
  588
+                                          'Deprecated, check for __IPYTHON__'
  589
+
576 590
         self.builtin_trap = BuiltinTrap(shell=self)
577 591
 
578 592
     def init_inspector(self):
5  IPython/core/tests/test_interactiveshell.py
@@ -271,3 +271,8 @@ def test_1(self):
271 271
         """
272 272
         cmd = ur'''python -c "'åäö'"   '''
273 273
         _ip.shell.system_raw(cmd)
  274
+
  275
+
  276
+def test__IPYTHON__():
  277
+    # This shouldn't raise a NameError, that's all
  278
+    __IPYTHON__
6  IPython/frontend/terminal/interactiveshell.py
@@ -341,9 +341,6 @@ def interact(self, display_banner=None):
341 341
 
342 342
         more = False
343 343
 
344  
-        # Mark activity in the builtins
345  
-        __builtin__.__dict__['__IPYTHON__active'] += 1
346  
-
347 344
         if self.has_readline:
348 345
             self.readline_startup_hook(self.pre_readline)
349 346
             hlen_b4_cell = self.readline.get_current_history_length()
@@ -413,9 +410,6 @@ def interact(self, display_banner=None):
413 410
                     hlen_b4_cell = \
414 411
                         self._replace_rlhist_multiline(source_raw, hlen_b4_cell)
415 412
 
416  
-        # We are off again...
417  
-        __builtin__.__dict__['__IPYTHON__active'] -= 1
418  
-
419 413
         # Turn off the exit flag, so the mainloop can be restarted if desired
420 414
         self.exit_now = False
421 415
 
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.