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

Update Parallel Magics and Exception Display #1893

Merged
merged 27 commits into from Jun 12, 2012

Conversation

Projects
None yet
4 participants
@minrk
Member

minrk commented Jun 10, 2012

Based on feedback from @fperez, a few small changes to parallel exception handling and magics:

Exception changes:

  • apply_requests trigger showtraceback machinery, so apply errors are as pretty as execute ones
  • InteractiveShell.showtraceback handles RemoteErrors, so it only draws the remote traceback, rather than the unhelpful local one.

Magics changes:

  • removed parallelmagic extension
  • creating a Client implies activate of a lazily-evaluated directview on all engines
  • can activate Magics on multiple views with different suffixes:
eall = rc.activate('all', 'all')
e0 = rc.activate(0, '0')
%pxall a=5
%px0 print a
  • add %pxconfig magic for changing default block/targets for a collection of magics
  • add targets arg to %%px cell magic
  • %result renamed to %pxresult for consistency (%result kept for bw compat)
  • %pxresult now only draws most recent result, but accepts all the output-formatting args of %%px
  • add --out arg to %%px for storing the AsyncResult object in the user_ns

I need to do another pass on docs and tests, but I think it's mostly there.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Jun 10, 2012

Member

Sweet!! Reviewing now... @ellisonbg and @jonathan-taylor, you'll want to have a look at this. During an intense session this week with @jonathan-taylor using the notebook to debug some parallel statistical code, I kept making mental notes and thinking about how we could make the process smoother and easier. Min and I then had a very productive, long discussion on Thursday where we bashed out all these ideas and this is the result. I'm digging into the review now, but having your thoughts would be great too.

Overall I think Min and I felt these changes would make big improvements to the fluidity of the experience and would open up good patterns for using the system, let us know what you think.

Member

fperez commented Jun 10, 2012

Sweet!! Reviewing now... @ellisonbg and @jonathan-taylor, you'll want to have a look at this. During an intense session this week with @jonathan-taylor using the notebook to debug some parallel statistical code, I kept making mental notes and thinking about how we could make the process smoother and easier. Min and I then had a very productive, long discussion on Thursday where we bashed out all these ideas and this is the result. I'm digging into the review now, but having your thoughts would be great too.

Overall I think Min and I felt these changes would make big improvements to the fluidity of the experience and would open up good patterns for using the system, let us know what you think.

if not _loaded:
ip.register_magics(ParallelMagics)
_loaded = True
warn("Parallel Magics are no longer defined in an extension", DeprecationWarning)

This comment has been minimized.

@fperez

fperez Jun 10, 2012

Member

This should also say, "use import ..." so that users know what the new API is. Just a bit of user-friendly sugar.

@fperez

fperez Jun 10, 2012

Member

This should also say, "use import ..." so that users know what the new API is. Just a bit of user-friendly sugar.

This comment has been minimized.

@minrk

minrk Jun 10, 2012

Member

Sure, though I should note that the API hasn't changed. Nobody should have ever explicitly called %load_ext parallelmagic at any point, as it's always been totally redundant with activate. The only difference now is that activate is unnecessary as well, rather than being the only necessary step.

@minrk

minrk Jun 10, 2012

Member

Sure, though I should note that the API hasn't changed. Nobody should have ever explicitly called %load_ext parallelmagic at any point, as it's always been totally redundant with activate. The only difference now is that activate is unnecessary as well, rather than being the only necessary step.

This comment has been minimized.

@fperez

fperez Jun 11, 2012

Member

Agreed, I'm just being 'extra friendly' here with errors. It's fine if it stays as-is.

@fperez

fperez Jun 11, 2012

Member

Agreed, I'm just being 'extra friendly' here with errors. It's fine if it stays as-is.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Jun 10, 2012

Member

This looks very solid to me, awesome job. But I'm obviously biased, since we went over the design together, so I'll hold off until others have a chance to give their opinons as well...

We can't run test_pr right now b/c master has an actual failure, I'm going to try to fix that urgently so we can get back to a working master.

Member

fperez commented Jun 10, 2012

This looks very solid to me, awesome job. But I'm obviously biased, since we went over the design together, so I'll hold off until others have a chance to give their opinons as well...

We can't run test_pr right now b/c master has an actual failure, I'm going to try to fix that urgently so we can get back to a working master.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Jun 11, 2012

Member

Test results for commit b7dc35d (can't merge cleanly)
Platform: linux2

  • python2.7: OK

Not available for testing:

Member

fperez commented Jun 11, 2012

Test results for commit b7dc35d (can't merge cleanly)
Platform: linux2

  • python2.7: OK

Not available for testing:

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jun 11, 2012

Member

Test results for commit b7dc35d merged into master
Platform: darwin

  • python2.6: OK (libraries not available: cython matplotlib pygments pymongo qt rpy2 tornado wx wx.aui)
  • python2.7: Failed, log at https://gist.github.com/2909198 (libraries not available: rpy2 wx wx.aui)
  • python3.2: OK (libraries not available: cython matplotlib pymongo qt rpy2 wx wx.aui)

Not available for testing:

Member

minrk commented Jun 11, 2012

Test results for commit b7dc35d merged into master
Platform: darwin

  • python2.6: OK (libraries not available: cython matplotlib pygments pymongo qt rpy2 tornado wx wx.aui)
  • python2.7: Failed, log at https://gist.github.com/2909198 (libraries not available: rpy2 wx wx.aui)
  • python3.2: OK (libraries not available: cython matplotlib pymongo qt rpy2 wx wx.aui)

Not available for testing:

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Jun 11, 2012

Member

Odd, I don't see the any of the 2.7 failures, these aren't all reload ones, are they?

Member

fperez commented Jun 11, 2012

Odd, I don't see the any of the 2.7 failures, these aren't all reload ones, are they?

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jun 11, 2012

Member

Yes, they are all autoreload - they are all the same error, where distutils does an if foo is Extension test, but foo is the autoreloaded Extension, and Extension is the original, so is fails.

Member

minrk commented Jun 11, 2012

Yes, they are all autoreload - they are all the same error, where distutils does an if foo is Extension test, but foo is the autoreloaded Extension, and Extension is the original, so is fails.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Jun 11, 2012

Member

Do you want to disable those tests as a separate commit? That could go into master quickly, so we can really trust test_pr everywhere... I'd rather have a few less tests than a recurrent failure...

Member

fperez commented Jun 11, 2012

Do you want to disable those tests as a separate commit? That could go into master quickly, so we can really trust test_pr everywhere... I'd rather have a few less tests than a recurrent failure...

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jun 11, 2012

Member

Done: #1911

Member

minrk commented Jun 11, 2012

Done: #1911

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Jun 11, 2012

Member

OK, merged and made #1912 to track their isolated re-enabling. Thanks!

Member

fperez commented Jun 11, 2012

OK, merged and made #1912 to track their isolated re-enabling. Thanks!

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Jun 12, 2012

Member

Test results for commit b7dc35d merged into master
Platform: linux2

  • python2.7: OK (libraries not available: rpy2 tornado)
  • python3.1: OK (libraries not available: cython matplotlib numpy pymongo qt rpy2 wx wx.aui zmq)
  • python3.2: OK (libraries not available: cython pymongo rpy2 wx wx.aui)

Not available for testing: python2.6

Member

takluyver commented Jun 12, 2012

Test results for commit b7dc35d merged into master
Platform: linux2

  • python2.7: OK (libraries not available: rpy2 tornado)
  • python3.1: OK (libraries not available: cython matplotlib numpy pymongo qt rpy2 wx wx.aui zmq)
  • python3.2: OK (libraries not available: cython pymongo rpy2 wx wx.aui)

Not available for testing: python2.6

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Jun 12, 2012

Member

(was just using this to test the post_pr_test.py script)

Member

takluyver commented Jun 12, 2012

(was just using this to test the post_pr_test.py script)

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Jun 12, 2012

Member

This is such a big improvement to the parallel machinery that I'm tempted to merge it now, so that we can get more feedback on the whole thing as soon as possible in master. We can always refine a bit later, but the code looks clean, the tests all pass and I'm pretty convinced the API and experience are a big step forward. Thoughts?

Member

fperez commented Jun 12, 2012

This is such a big improvement to the parallel machinery that I'm tempted to merge it now, so that we can get more feedback on the whole thing as soon as possible in master. We can always refine a bit later, but the code looks clean, the tests all pass and I'm pretty convinced the API and experience are a big step forward. Thoughts?

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jun 12, 2012

Member

I've got a couple more tweaks, tests, and a doc pass almost done, but it should be ready by the end of today.

Member

minrk commented Jun 12, 2012

I've got a couple more tweaks, tests, and a doc pass almost done, but it should be ready by the end of today.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Jun 12, 2012

Member

Great, ping when ready. I'm really excited to see this go in.

Member

fperez commented Jun 12, 2012

Great, ping when ready. I'm really excited to see this go in.

minrk added some commits Jun 8, 2012

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jun 12, 2012

Member

Okay, should be ready. Running test_pr now.

Further changes:

  • apply_requests also trigger busy/idle status messages
  • idle message is used to signal that outputs are ready (should help some of the occasionally-failing tests)
Member

minrk commented Jun 12, 2012

Okay, should be ready. Running test_pr now.

Further changes:

  • apply_requests also trigger busy/idle status messages
  • idle message is used to signal that outputs are ready (should help some of the occasionally-failing tests)
@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jun 12, 2012

Member

added %pxconfig --[no-]verbose, per ML discussion.

Running test_pr, which will certainly fail due to #1916, but these tests should be okay

Member

minrk commented Jun 12, 2012

added %pxconfig --[no-]verbose, per ML discussion.

Running test_pr, which will certainly fail due to #1916, but these tests should be okay

@ellisonbg

This comment has been minimized.

Show comment
Hide comment
@ellisonbg

ellisonbg Jun 12, 2012

Member

But the eliding doesn't help if there is useful output below the list
of engines that you want to see. So I think the --[no]-verbose
options will be useful.

On Mon, Jun 11, 2012 at 10:32 PM, Min RK
reply@reply.github.com
wrote:

added %pxconfig --[no-]verbose, per ML discussion.

Running test_pr, which will certainly fail due to #1916, but these tests should be okay


Reply to this email directly or view it on GitHub:
#1893 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

Member

ellisonbg commented Jun 12, 2012

But the eliding doesn't help if there is useful output below the list
of engines that you want to see. So I think the --[no]-verbose
options will be useful.

On Mon, Jun 11, 2012 at 10:32 PM, Min RK
reply@reply.github.com
wrote:

added %pxconfig --[no-]verbose, per ML discussion.

Running test_pr, which will certainly fail due to #1916, but these tests should be okay


Reply to this email directly or view it on GitHub:
#1893 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jun 12, 2012

Member

Test results for commit 59f329c merged into master
Platform: darwin

Not available for testing: python3.1

Member

minrk commented Jun 12, 2012

Test results for commit 59f329c merged into master
Platform: darwin

Not available for testing: python3.1

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jun 12, 2012

Member

@ellisonbg I don't know what you mean - there's no difference in size between 8 engines and 800. The "Parallel execution on..." wil always fit comfortably on one line. But I added the verbose flag, and all parallel tests pass, despite the appearance of the above report from test_pr.

Member

minrk commented Jun 12, 2012

@ellisonbg I don't know what you mean - there's no difference in size between 8 engines and 800. The "Parallel execution on..." wil always fit comfortably on one line. But I added the verbose flag, and all parallel tests pass, despite the appearance of the above report from test_pr.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Jun 12, 2012

Member

Is no-verbose now the default, or verbose?

Member

fperez commented Jun 12, 2012

Is no-verbose now the default, or verbose?

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jun 12, 2012

Member

As you guys requested on the list, verbose is False by default.

Member

minrk commented Jun 12, 2012

As you guys requested on the list, verbose is False by default.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Jun 12, 2012

Member

Great, just checking. My vote is then to merge this. It's very valuable and I think from now on it's diminishing returns to keep it in review... What do you think?

Member

fperez commented Jun 12, 2012

Great, just checking. My vote is then to merge this. It's very valuable and I think from now on it's diminishing returns to keep it in review... What do you think?

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jun 12, 2012

Member

I don't have any further plans on it anymore, so I'm fine with merge.

Member

minrk commented Jun 12, 2012

I don't have any further plans on it anymore, so I'm fine with merge.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Jun 12, 2012

Member

OK, let's go ahead, so we have a chance to see its effect on master for a day or so before the beta. The closer the beta is to a real RC, the better off we'll be.

Awesome job, and thanks for the patience with all this went through!

Member

fperez commented Jun 12, 2012

OK, let's go ahead, so we have a chance to see its effect on master for a day or so before the beta. The closer the beta is to a real RC, the better off we'll be.

Awesome job, and thanks for the patience with all this went through!

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Jun 12, 2012

Member

Merging now.

Member

fperez commented Jun 12, 2012

Merging now.

fperez added a commit that referenced this pull request Jun 12, 2012

Merge pull request #1893 from minrk/compositeerr
Update Parallel Magics and Exception Display

Based on feedback from @fperez, a few small changes to parallel exception handling and magics:

Exception changes:

* apply_requests trigger showtraceback machinery, so apply errors are as pretty as execute ones
* InteractiveShell.showtraceback handles RemoteErrors, so it only draws the remote traceback, rather than the unhelpful local one.

Magics changes:

* removed parallelmagic extension
* creating a Client *implies* activate of a lazily-evaluated directview on all engines
* can activate Magics on multiple views with different suffixes:

```python
eall = rc.activate('all', 'all')
e0 = rc.activate(0, '0')
%pxall a=5
%px0 print a
```

* add %pxconfig magic for changing default block/targets for a collection of magics
* add targets arg to %%px cell magic
* %result renamed to %pxresult for consistency (%result kept for bw compat)
* %pxresult now only draws most recent result, but accepts all the output-formatting args of %%px
* add --out arg to %%px for storing the AsyncResult object in the user_ns
* changed %px to not be verbose by default, and added verbosity control to %pxconfig.

@fperez fperez merged commit 60e6629 into ipython:master Jun 12, 2012

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Jun 12, 2012

Member

On Jun 11, 2012, at 23:36, Fernando Perezreply@reply.github.com wrote:

OK, let's go ahead, so we have a chance to see its effect on master for a day or so before the beta. The closer the beta is to a real RC, the better off we'll be.

Certainly.

Awesome job, and thanks for the patience with all this went through!

The code is better for it. And besides, this is largely your suggestions, I just typed it up.


Reply to this email directly or view it on GitHub:
#1893 (comment)

Member

minrk commented Jun 12, 2012

On Jun 11, 2012, at 23:36, Fernando Perezreply@reply.github.com wrote:

OK, let's go ahead, so we have a chance to see its effect on master for a day or so before the beta. The closer the beta is to a real RC, the better off we'll be.

Certainly.

Awesome job, and thanks for the patience with all this went through!

The code is better for it. And besides, this is largely your suggestions, I just typed it up.


Reply to this email directly or view it on GitHub:
#1893 (comment)

@minrk minrk deleted the minrk:compositeerr branch Mar 31, 2014

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

Merge pull request #1893 from minrk/compositeerr
Update Parallel Magics and Exception Display

Based on feedback from @fperez, a few small changes to parallel exception handling and magics:

Exception changes:

* apply_requests trigger showtraceback machinery, so apply errors are as pretty as execute ones
* InteractiveShell.showtraceback handles RemoteErrors, so it only draws the remote traceback, rather than the unhelpful local one.

Magics changes:

* removed parallelmagic extension
* creating a Client *implies* activate of a lazily-evaluated directview on all engines
* can activate Magics on multiple views with different suffixes:

```python
eall = rc.activate('all', 'all')
e0 = rc.activate(0, '0')
%pxall a=5
%px0 print a
```

* add %pxconfig magic for changing default block/targets for a collection of magics
* add targets arg to %%px cell magic
* %result renamed to %pxresult for consistency (%result kept for bw compat)
* %pxresult now only draws most recent result, but accepts all the output-formatting args of %%px
* add --out arg to %%px for storing the AsyncResult object in the user_ns
* changed %px to not be verbose by default, and added verbosity control to %pxconfig.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment