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

Transform code before %prun/%%prun runs #4008

Merged
merged 2 commits into from Aug 13, 2013

Conversation

jasongrout
Copy link
Member

This makes it so that prun works with IPython-specific syntax (e.g., %prun %logon). This mirrors the change @fperez made in commit 3ac024c for the timeit magic.

@takluyver
Copy link
Member

Should it also apply the AST transformers? It's largely academic from IPython's point of view, as we don't have any ourselves, but Sympy & Sage might. I don't know how easy that is with the profiler code, though.

Also, should there be/can there be a test for this?

@jasongrout
Copy link
Member Author

At the moment Sage doesn't have AST transformers. I think the code modifications are a bit more to do the AST transforms, because currently you run the code from a string. And the same change should probably be made to timeit if it is done here.

A test seems fair enough. Is there a test for the %timeit change I could modify? I didn't see one in Fernando's commit.

@takluyver
Copy link
Member

%timeit and %time already do AST transforms.

IPython.core.tests.test_magic.test_timeit_special_syntax appears to be the relevant test. Tests for AST transformations are in IPython.core.tests.test_interactiveshell.

@takluyver
Copy link
Member

But yes, it's probably OK to leave AST transforms out of %prun for the moment - it would be a fair bit of extra complexity, and it can always be done in another PR. Pinging @asmeurer - does Sympy use any AST transforms?

@asmeurer
Copy link
Contributor

No, I never got around to updating the code.

@asmeurer
Copy link
Contributor

The automatic symbols and int sympification from https://github.com/sympy/sympy/blob/master/sympy/interactive/session.py are what need to be updated.

I don't really understand what the question is here for me.

@takluyver
Copy link
Member

Jason is updating %prun to handle syntax transformations, but I think we're leaving AST transformations out for the time being. I just wanted to keep you in the loop of where AST transformations do and don't work at present.

@jasongrout
Copy link
Member Author

Okay, I think I did the test correctly. I wasn't sure what the skipif decorator on the other prun test was for, but it looked important, so I copied it over. Is it needed?

@takluyver
Copy link
Member

Yep, that looks sound. I'll merge once Travis gives it the OK. Thanks!

Yes, the skipif decorator is needed. profile may not always be there, for some license-y reasons I don't know the details of.

@jasongrout
Copy link
Member Author

Is there stable and dev branches right now? It would be great if this was included in the next bugfix release (it would eliminate a patch we are applying to IPython in Sage). Or is the plan to just forge ahead with one release branch?

@takluyver
Copy link
Member

Yes, there's a 1.x branch, and I'd be happy for this to get backported. But we're also aiming to have faster major releases, so 2.0 will probably be this winter.

Merging to master, then I'll work out whether & how to backport.

takluyver added a commit that referenced this pull request Aug 13, 2013
Transform code before %prun/%%prun runs
@takluyver takluyver merged commit 59d4731 into ipython:master Aug 13, 2013
@jasongrout jasongrout deleted the prun-transform branch August 13, 2013 18:36
@bfroehle
Copy link
Contributor

@takluyver I set the backport-1.1 label, which I think is the appropriate way for us to mark this as something to apply to the next bugfix release.

@takluyver
Copy link
Member

I'll just do it.

takluyver added a commit that referenced this pull request Aug 13, 2013
This makes it so that prun works with IPython-specific syntax (e.g., `%prun %logon`).  This mirrors the change @fperez made in commit 3ac024c for the timeit magic.
@takluyver
Copy link
Member

Backported.

@ivanov
Copy link
Member

ivanov commented Aug 13, 2013

@bfroehle thanks for tagging it with backport, and yes, as @takluyver did, it's nice to get these backports out of the way so we have less work to do for those bug fix releases. Cherry picking a squashed version of a PR is fair game, and we have a script it tools/backport_pr.py that does just that in a simplified manner just with backport_pr.py 1.x 4008 to backport #4008 onto the 1.x branch

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Transform code before %prun/%%prun runs
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

Successfully merging this pull request may close these issues.

None yet

5 participants