embed(): Default to the future compiler flags of the calling frame. #2096

Merged
merged 3 commits into from Jul 18, 2012

Conversation

Projects
None yet
3 participants
@bfroehle
Contributor

bfroehle commented Jul 4, 2012

As discussed on the mailing list by Joon Ro:

I'm using ipython 0.13. I just found that even though my main script has from __future__ import division, the embedded ipython session I still get 1/2 = 0

It has caused me some confusion and I was wondering if this is a bug.

The attached pull request changes the behavior of embed to, by default, use the future compiler flags from the calling frame.

@takluyver

This comment has been minimized.

Show comment Hide comment
@takluyver

takluyver Jul 4, 2012

Member

Kudos for getting onto this so quickly. That looks like exactly the approach I was thinking of.

Can we have a test for this - even if it's not possible to do as a fully automated test, we should have a script in docs/examples/tests/embed that we can re-run to test this.

Member

takluyver commented Jul 4, 2012

Kudos for getting onto this so quickly. That looks like exactly the approach I was thinking of.

Can we have a test for this - even if it's not possible to do as a fully automated test, we should have a script in docs/examples/tests/embed that we can re-run to test this.

@bfroehle

This comment has been minimized.

Show comment Hide comment
@bfroehle

bfroehle Jul 4, 2012

Contributor

Aha! I didn't know of the existence of docs/examples/tests/embed. I don't see a way to test this directly, but I did add two quick test script for testing from __future__ import division. Note that this is only relevant in Python 2, but will work in Python 3.

Contributor

bfroehle commented Jul 4, 2012

Aha! I didn't know of the existence of docs/examples/tests/embed. I don't see a way to test this directly, but I did add two quick test script for testing from __future__ import division. Note that this is only relevant in Python 2, but will work in Python 3.

@bfroehle

This comment has been minimized.

Show comment Hide comment
@bfroehle

bfroehle Jul 4, 2012

Contributor

Aha! I didn't know of the existence of docs/examples/tests/embed. I don't see a way to test this directly, but I did add two quick test script for testing from __future__ import division. Note that this test is a no-op in Python 3, but testing in Python 2 should be sufficient.

Contributor

bfroehle commented Jul 4, 2012

Aha! I didn't know of the existence of docs/examples/tests/embed. I don't see a way to test this directly, but I did add two quick test script for testing from __future__ import division. Note that this test is a no-op in Python 3, but testing in Python 2 should be sufficient.

@takluyver

This comment has been minimized.

Show comment Hide comment
@takluyver

takluyver Jul 5, 2012

Member

Checked the script in 2.7 and 3.2. Looks OK to me, but I'll let someone else give it a once over.

Member

takluyver commented Jul 5, 2012

Checked the script in 2.7 and 3.2. Looks OK to me, but I'll let someone else give it a once over.

@joonro

This comment has been minimized.

Show comment Hide comment
@joonro

joonro Jul 5, 2012

Contributor

@bfroehle Thanks a lot for this!

Contributor

joonro commented Jul 5, 2012

@bfroehle Thanks a lot for this!

@bfroehle

View changes

IPython/core/compilerop.py
+# Roughtly equal to PyCF_MASK | PyCF_MASK_OBSOLETE as defined in pythonrun.h,
+# this is used as a bitmask to extract future-related code flags.
+PyCF_MASK = functools.reduce(operator.or_,
+ (f.compiler_flag for f in codeop._features))

This comment has been minimized.

Show comment Hide comment
@bfroehle

bfroehle Jul 15, 2012

Contributor

Should we not rely on codeop._features and instead produce this as:

PyCF_MASK = functools.reduce(operator.or_,
                             (getattr(__future__, fname).compiler_flag
                              for fname in __future__.all_feature_names))

For reference, codeop._features is implemented as

# codeop.py:

import __future__

_features = [getattr(__future__, fname)
             for fname in __future__.all_feature_names]
@bfroehle

bfroehle Jul 15, 2012

Contributor

Should we not rely on codeop._features and instead produce this as:

PyCF_MASK = functools.reduce(operator.or_,
                             (getattr(__future__, fname).compiler_flag
                              for fname in __future__.all_feature_names))

For reference, codeop._features is implemented as

# codeop.py:

import __future__

_features = [getattr(__future__, fname)
             for fname in __future__.all_feature_names]

This comment has been minimized.

Show comment Hide comment
@takluyver

takluyver Jul 18, 2012

Member

I'd side with the version using __future__, because codeop._features is undocumented, so it could theoretically be removed or renamed in a future version of Python.

@takluyver

takluyver Jul 18, 2012

Member

I'd side with the version using __future__, because codeop._features is undocumented, so it could theoretically be removed or renamed in a future version of Python.

@bfroehle

View changes

IPython/frontend/terminal/embed.py
@@ -162,7 +162,7 @@ def __call__(self, header='', local_ns=None, module=None, dummy=None,
print self.exit_msg
def mainloop(self, local_ns=None, module=None, stack_depth=0,
- display_banner=None, global_ns=None):
+ display_banner=None, global_ns=None, flags=None):

This comment has been minimized.

Show comment Hide comment
@bfroehle

bfroehle Jul 18, 2012

Contributor

In terms of naming, is flags okay? Or should we make it compiler_flags ?

@bfroehle

bfroehle Jul 18, 2012

Contributor

In terms of naming, is flags okay? Or should we make it compiler_flags ?

This comment has been minimized.

Show comment Hide comment
@takluyver

takluyver Jul 18, 2012

Member

compiler_flags, I think. Explicit is better than implicit, and this parameter seems like something few people will need, so there's no need to make it quick to type.

@takluyver

takluyver Jul 18, 2012

Member

compiler_flags, I think. Explicit is better than implicit, and this parameter seems like something few people will need, so there's no need to make it quick to type.

@bfroehle

This comment has been minimized.

Show comment Hide comment
@bfroehle

bfroehle Jul 18, 2012

Contributor

Made the changes, rebased, and squashed into two commits (feature, tests).

I ended up naming the option compile_flags which matches get_ipython().compile.flags nicely.

Contributor

bfroehle commented Jul 18, 2012

Made the changes, rebased, and squashed into two commits (feature, tests).

I ended up naming the option compile_flags which matches get_ipython().compile.flags nicely.

@bfroehle

This comment has been minimized.

Show comment Hide comment
@bfroehle

bfroehle Jul 18, 2012

Contributor

I think this is ready to go.

Contributor

bfroehle commented Jul 18, 2012

I think this is ready to go.

@takluyver

This comment has been minimized.

Show comment Hide comment
@takluyver

takluyver Jul 18, 2012

Member

I don't think it's currently possible to pass compile_flags to embed(), because its kwargs go to the constructor, not to mainloop(). If we're going to offer the parameter, we should probably make sure it's useful. I'd pass it through from __call__() to mainloop(), and grab it in embed() like we do for header. Then a quick test script that checks the parameter would be good as well.

Also, mainloop() has a docstring that describes parameters, so the new parameter should be added to that.

Member

takluyver commented Jul 18, 2012

I don't think it's currently possible to pass compile_flags to embed(), because its kwargs go to the constructor, not to mainloop(). If we're going to offer the parameter, we should probably make sure it's useful. I'd pass it through from __call__() to mainloop(), and grab it in embed() like we do for header. Then a quick test script that checks the parameter would be good as well.

Also, mainloop() has a docstring that describes parameters, so the new parameter should be added to that.

@bfroehle

This comment has been minimized.

Show comment Hide comment
@bfroehle

bfroehle Jul 18, 2012

Contributor

Wow that was bad of me. I never tested passing compile_flags as a keyword argument... just assumed that it would work.

Okay added documentation and now treat compile_flags like header in embed.

Contributor

bfroehle commented Jul 18, 2012

Wow that was bad of me. I never tested passing compile_flags as a keyword argument... just assumed that it would work.

Okay added documentation and now treat compile_flags like header in embed.

@takluyver

This comment has been minimized.

Show comment Hide comment
@takluyver

takluyver Jul 18, 2012

Member

OK, that's looking good now. Assuming you've run the test scripts to check that it's all working as it should, I think you can merge when you're ready.

Member

takluyver commented Jul 18, 2012

OK, that's looking good now. Assuming you've run the test scripts to check that it's all working as it should, I think you can merge when you're ready.

bfroehle added a commit that referenced this pull request Jul 18, 2012

Merge pull request #2096 from bfroehle/embed_compiler_flags
embed(): Default to the compile flags of the calling frame.

This means that if a script contains:

    from __future__ import division
    from IPython import embed
    embed()

... then the embedded IPython session will also use the future division,
i.e., 1/2 evaluates to 0.5.

In addition, the user can pass the keyword argument `compile_flags` to
embed to override the default inheritance behavior, e.g.,

    embed(compile_flags=__future__.print_function.compiler_flag)

Multiple compile flags can be combined with the bitwise or operator.

@bfroehle bfroehle merged commit 54e6c7d into ipython:master Jul 18, 2012

bfroehle added a commit to bfroehle/ipython that referenced this pull request Aug 3, 2012

Fix regression in embed() from pull-request #2096.
With certain sets of arguments `compile_flags` might be left as `None`. This
caused IPython to internally raise a TypeError when it tried to do a
bitwise or between `shell.compile.flags` and `PyCF_ONLY_AST` in
`CachingCompiler.ast_parse`.

The regression was introduced in:
  b70ac12 embed(): Default to the future compile flags of the calling frame.

Carreau added a commit that referenced this pull request Aug 4, 2012

Merge pull request #2245 from bfroehle/qsnake_fix
Fix regression in embed() from pull-request #2096.

@bfroehle bfroehle referenced this pull request Aug 12, 2012

Closed

Regression in .embed() #2243

Carreau added a commit to Carreau/ipython that referenced this pull request Sep 5, 2012

Fix regression in embed() from pull-request #2096.
With certain sets of arguments `compile_flags` might be left as `None`. This
caused IPython to internally raise a TypeError when it tried to do a
bitwise or between `shell.compile.flags` and `PyCF_ONLY_AST` in
`CachingCompiler.ast_parse`.

The regression was introduced in:
  b70ac12 embed(): Default to the future compile flags of the calling frame.

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #2096 from bfroehle/embed_compiler_flags
embed(): Default to the compile flags of the calling frame.

This means that if a script contains:

    from __future__ import division
    from IPython import embed
    embed()

... then the embedded IPython session will also use the future division,
i.e., 1/2 evaluates to 0.5.

In addition, the user can pass the keyword argument `compile_flags` to
embed to override the default inheritance behavior, e.g.,

    embed(compile_flags=__future__.print_function.compiler_flag)

Multiple compile flags can be combined with the bitwise or operator.

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Fix regression in embed() from pull-request #2096.
With certain sets of arguments `compile_flags` might be left as `None`. This
caused IPython to internally raise a TypeError when it tried to do a
bitwise or between `shell.compile.flags` and `PyCF_ONLY_AST` in
`CachingCompiler.ast_parse`.

The regression was introduced in:
  b70ac12 embed(): Default to the future compile flags of the calling frame.

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #2245 from bfroehle/qsnake_fix
Fix regression in embed() from pull-request #2096.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment