Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

shell access using ! will not fill class or function scope vars #1878

Closed
steverweber opened this issue Jun 7, 2012 · 22 comments
Closed

shell access using ! will not fill class or function scope vars #1878

steverweber opened this issue Jun 7, 2012 · 22 comments
Milestone

Comments

@steverweber
Copy link

the ! to access the shell is not filling scoped variables.

def test():
  defvar='true'
  !echo def Test worked? {defvar}
test()
# def Test worked? {defvar}

class cTest:
   classvar='true'
   def test(self):
      !echo class Test worked? {self.classvar}
ctest = cTest()
ctest.test()

I feel this should be rather high priority since this issue makes writing Ipython shell scripts a PITA..

IRC dump

[10:45] == steverweber [81617c7d@gateway/web/freenode/ip.129.97.124.125] has joined #ipython
[10:46] <steverweber> using ipython3
[10:46] <steverweber> def test():
[10:46] <steverweber>     defvar='true'
[10:46] <steverweber>     !echo def Test worked? {defvar}
[10:46] <steverweber> test()
[10:47] <steverweber> :( def Test worked? {defvar}
[10:48] <steverweber> how do i pass function or class scope variables to the shell?
[10:50] <btiffin> Try  !echo "def Test worked? {defvar}"
[10:52] <steverweber> same.
[10:53] <btiffin> Hmm, worked here.  Even without quotes.
[10:54] <steverweber> r u running trunk?
[10:54] <steverweber> ill try the latest release.. brb
[10:55] <btiffin> 0.12 from the Fedora 16 repos
[10:56] <steverweber> one more test plez
[10:56] <steverweber> ---------
[10:56] <steverweber> class cTest:
[10:56] <steverweber>   classvar='true'
[10:56] <steverweber>     def test(self):
[10:57] <steverweber>     !echo class Test worked? {self.classvar}
[10:57] <steverweber> ctest = cTest()
[10:57] <steverweber> ctest.test()
[10:57] <steverweber> -----------
[11:01] <jtaylor> you could just do os.system("echo class Test worked? " + defvar)
[11:02] <steverweber> yes... but it would be nice if the ! worked...
[11:04] <jtaylor> not sure how that works, I guess it just picks the variable from the global namespace
[11:04] <jtaylor> so doesn#T work for local ones
[11:05] <steverweber> i think btiffin was mistaken and never preformed the function scope test
[11:06] <steverweber> could you confirm that it as well will not work\
[11:07] == zendeavor has changed nick to meskaprune
[11:07] <jtaylor> it doesn'T work with git, I also don't think it works with 0.12.1 but I can try
[11:07] <steverweber> i tried latest.. it dont work
[11:08] == meskaprune has changed nick to meskaHAIRYLEGS
[11:09] <jtaylor> doesn't work in 0.12.1
[11:09] == meskaHAIRYLEGS has changed nick to automage
[11:10] <steverweber> adding as issue to git hub
[11:15] <btiffin> steverweber: could be, plus I'm 2.7.3 and not 3, so  I'm likely chucking red herrings
[11:15] <steverweber> thanks
@takluyver
Copy link
Member

Looking at the code, it should work (see var_expand in IPython.core.interactiveshell), although self is deliberately excluded for some reason.

I guess we're getting the depth to look through the stack frame wrong. We look 2 frames above system_raw() or system_piped(), but it should only be 1. That sort of thing is always fragile.

The good news is, it should be an easy fix. Would you like to have a go at it?

@minrk
Copy link
Member

minrk commented Jun 7, 2012

I don't think this is as easy as that. var_expand expands variables in user_ns, it does not have access to the calling scope.

@takluyver
Copy link
Member

...the var_expand I'm looking at does have access to the calling scope. ;-) See this line:

ns.update(sys._getframe(depth+1).f_locals)

I'll experiment with a fix now.

@takluyver
Copy link
Member

Should be fixed by PR #1879

@juliantaylor
Copy link
Contributor

this should probably be marked for a potential 0.12.2

@minrk
Copy link
Member

minrk commented Jun 7, 2012

I would give 0.12.2 very nearly 0% chance of existing.

@fperez
Copy link
Member

fperez commented Jun 8, 2012

@juliantaylor, unfortunately my estimate for 0.12.2 is about 1/2 of @minrk's ;)

@fperez
Copy link
Member

fperez commented Jun 8, 2012

ps - not to say that someone else couldn't do it, if it's necessary to keep the 12.04 LTS state in good health... But we simply won't be able to allocate any time to it.

@juliantaylor
Copy link
Contributor

its still useful to mark bugs affecting spread versions even if you do not intend to fix them.
It makes it easier to find savely back-portable issues for production systems.

e.g. how the bugs in debian works, they all have a version-found and version-fixed tag.

@fperez
Copy link
Member

fperez commented Jun 8, 2012

On Fri, Jun 8, 2012 at 12:54 AM, Julian Taylor
reply@reply.github.com
wrote:

its still useful to mark bugs affecting spread versions even if you do not intend to fix them.
It makes it easier to find savely back-portable issues for production systems.

Sure, no problem.

Carreau pushed a commit to Carreau/ipython that referenced this issue Jun 8, 2012
Correct stack depth for variable expansion in !system commands

Closes ipython#1878.
@Carreau
Copy link
Member

Carreau commented Jun 8, 2012

I added some time ago a backport-0.12.2 label, just in case. We can add it on the fly to PR/issues that should might be worth it.

@bfroehle
Copy link
Contributor

bfroehle commented Jun 8, 2012

This commit is causing iptest to fail:

$ iptest IPython.testing
............F.................S..S........................S
======================================================================
FAIL: Doctest: test_exampleip.txt
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/doctest.py", line 2201, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for test_exampleip.txt
  File "/home/bfroehle/src/ipython/IPython/testing/plugin/test_exampleip.txt", line 0

----------------------------------------------------------------------
File "/home/bfroehle/src/ipython/IPython/testing/plugin/test_exampleip.txt", line 29, in test_exampleip.txt
Failed example:
    get_ipython().system('echo $a')
Expected:
    hi
Got nothing

>>  raise self.failureException(self.format_failure(<StringIO.StringIO instance at 0x540cea8>.getvalue()))


----------------------------------------------------------------------
Ran 63 tests in 0.253s

FAILED (SKIP=3, failures=1)

@takluyver
Copy link
Member

Interesting, I'm not seeing that failure.

@juliantaylor
Copy link
Contributor

@takluyver the ubuntu daily build sees the failure too, see the ipython-testresults package:
https://launchpad.net/~jtaylor/+archive/ipython-dev/+packages

@takluyver
Copy link
Member

Oh, I've worked out why I'm not seeing it. The error's in a .txt
doctest file, which doesn't get copied when you do "python setup.py
install", so that test is only run if the tests import directly from
the source directory.

Have I mentioned before that I hate doctests? ;-) Debugging something
like this is a royal pain.

As an aside, shouldn't a test failure stop the build? IIRC, it needs a
"set -e" before the loop in debian/rules to make it take the error
seriously.

@juliantaylor
Copy link
Contributor

As an aside, shouldn't a test failure stop the build?

normally yes, but as the ipython testsuite fails since several weeks the build ignores it and instead just saves it for later consumption.
Else the daily build would end up being out of date for too long times due to a single failure.
The results are better displayed in your shiningpanda instance.

@takluyver
Copy link
Member

See PR #1895 for this test failure

I remember we talked before about setting up ShiningPanda to get the
results from launchpad. My feeling is that it's quite a bit of effort
(scraping a link to the latest version, manually unpacking the deb
file) for relatively little gain - another Python 2.7 test run on the
OS we already have the most coverage for. ShiningPanda's build slaves
are running Debian, and we now have them testing against 5 versions of
Python.

@fperez
Copy link
Member

fperez commented Jun 10, 2012

On Sun, Jun 10, 2012 at 7:52 AM, Thomas Kluyver
reply@reply.github.com
wrote:

Have I mentioned before that I hate doctests? ;-) Debugging something
like this is a royal pain.

I hear you... We've been moving more and more away from them, they
were a convenient way to be able to test straight up interactive
copy/paste sessions early on when we were trying to bootstrap the
IPython testing from nearly nothing. At the time, the ipython objects
were much harder to test programatically, due to bad apis, so this was
a necessary evil.

But now we can definitely move away from them more and more.

Don't feel compelled to preserve existing doctests at all costs: if a
given feature can be reliably tested and covered with a regular
test, I think it's OK to nuke an old doctest and replace it, given the
maintenance headaches they cause. So you have my full support if you
ever decide to do doctest deletion/replacement as warranted.

@steverweber
Copy link
Author

I did a pull of the latest code..

output of the test.ipy:

def test():
  defvar='true'
  !echo def Test worked? {defvar}
test()
# def Test worked? {defvar}

class cTest:
   classvar='true'
   def test(self):
      !echo class Test worked? {self.classvar}
ctest = cTest()
ctest.test()

is:

def Test worked? true
class Test worked? {self.classvar}

it seems that self.classvar is not being resolved.

Running test.ipy like this:
git clone https://github.com/ipython/ipython.git
python ./ipython/ipython.py -i -- test.ipy

python --version
Python 2.7.3

cd ipython
git log
Date:   Fri Aug 10 21:32:53 2012 -0700
    Merge pull request #2280 from ikn/master

also tried a git checkout 34f8b22
that was the fix commit.

@steverweber
Copy link
Author

reopen?

@Carreau Carreau reopened this Aug 14, 2012
@takluyver
Copy link
Member

Looking into it.

@takluyver
Copy link
Member

See PR #2302.

Carreau pushed a commit to Carreau/ipython that referenced this issue Sep 5, 2012
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this issue Nov 3, 2014
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this issue Nov 3, 2014
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this issue Nov 3, 2014
Correct stack depth for variable expansion in !system commands

Closes ipython#1878.
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this issue Nov 3, 2014
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this issue Nov 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants