Initial Code to reduce parallel.Client caching

[jankatins]

See #1131 for the discussion

Up to now it only adds a purge_client_cache() method and fixes two minor bugs.

This is probably not merge ready!

[travisbot]

This pull request passes (merged 41d3f075 into f4616c9).

[minrk]

Can you tweak it so that purge_hub_results and purge_local_results are called by local/remote switches in purge_results? That way there can be a single call for dropping everything, and a single docstring for people to look at for discarding unused data, whether it be on the Client or on the Hub.

[jankatins]

I would argue against this: it would be confusing to have two different "interfaces" (hub "results" and client "cache") merged into in here and that replacing the original one:

What would purge_results(local=True, hub=False, jobs=[msg_ids...]) do? Or remove some kwargs, so that's only a switch between purge everything local and everything on the hub? Would this mean errors for users who update?

I would be more in favor to add a deprecation warning to the old purge_results method (I've actually looked if there are any deprecateddecorators used in IPython, but couldn't find anything).

Anyway: If you want to have a certain interface I have no problem implementing it. :-)

[minrk]

There are two reasons for this:

1. users are already calling purge_results as it is now, expecting it to do the the functionality you are adding here, and being disappointed.
2. purging the local cache should absolutely support purging subsets, just like the remote purge does. The local cache purge should support everything the remote one does (by ID, by engine ID, dump all).

So the choice is either

A) two methods with identical signatures for purging local and remote result caches
B) one method that does both / either based on args.

Honestly, I think both, where the existing purge_results() wraps the local/remote methods with default args that reproduces current behavior) makes the most sense. Further, the two names should reflect that they are simply remote/local versions of the same call (purge_results_on_hub and purge_client_caches do not accurately reflect this symmetry, perhaps purge_local_results and purge_hub_results).

The Hub is a cache of finished results, just like the local dict. It just happens to be remote.

Purging results is really just saying "I'm done with this data, and I don't want it taking up resources anymore". It's only a question of whose resources you want to stop taking up. The Client's, the Hub's, or both? I can think of no reason that dealing with the two should be different.

What would purge_results(local=True, hub=False, jobs=[msg_ids...]) do?

It should remove just those IDs from the local cache.

If you want to keep the current call (purge local cache, history and all) as something like client.forget_everything(), that's fine too. I imagine it is something people will want often enough to get its own call (I certainly want it when I do long running benchmarks).

[jankatins]

Ok, will do.

After trying to understand the client-controller connections I would actually rename purge_hub_results() to purge_controller_results() so that the terminology "hub" (or scheduler) doesn't even show up: you use a cluster with a controller and engines and the client connects to the controller.

So my idea would be

• client.purge_all_caches() # purges local and remote result caches and the history, no arguments
• client.purge_local_results(...all or per ids...) # like the current purge_results interface
• client.purge_controller_results(...all or per ids) # just a rename
• client.purge_results(...all or per ids...) # convenience method for deleting both on local and on remote -> API Change as it deletes from local too.

Also change the docstrings to "purge the cached results" so that it is clear that it is a cache of already computed results.

[minrk]

On Wed, Aug 29, 2012 at 2:40 PM, JanSchulz notifications@github.com wrote:

Ok, will do.

After trying to understand the client-controller connections I would
actually rename purge_hub_results() to purge_controller_results() so that
the terminology "hub" (or scheduler) doesn't even show up: you use a
cluster with a controller and engines and the client connects to the
controller.

I don't agree on this one. The Controller is really a deprecated notion
from the old Twisted days. What we used to call a 'controller' is really a
collection of loosely associated processes - one Hub and several
Schedulers. The coupling of Hub/Scheduler lives entirely in the
ipcontroller startup script. Ultimately, it will be possible to add and
remove schedulers, which will be necessary for scaling to large numbers of
engines, and implementing better broadcast. I think the 'Controller' term
is actively detrimental to understanding the IPython cluster, and is what
should go away, not Hub.

So my idea would be

• client.purge_all_caches() # purges local and remote result caches
  and the history, no arguments
• client.purge_local_results(...all or per ids...) # like the current
  purge_results interface
• client.purge_controller_results(...all or per ids) # just a rename
• client.purge_results(...all or per ids...) # convenience method for
  deleting both on local and on remote -> API Change as it deletes from local
  too.

Also change the docstrings to "purge the cached results" so that it is
clear that it is a cache of already computed results.

Sounds good, with two minor adjustments:

1. s/controller/hub
2. rename purge_all_caches to something more generic/destructive like
   purge_everything. 'caches' is not accurate because it purges more than
   caches. It's actually removing the history and disabling a security
   feature, neither of which are accurately described as caches.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2340#issuecomment-8142232.

[jankatins]

All Tests which access the hub history fail with an error. I've no idea why :-(

======================================================================
ERROR: test_purge_hub_results (IPython.parallel.tests.test_client.TestClient)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\data\jschulz\workspace\ipython\IPython\parallel\tests\test_client.py", line 412, in test_purge_hub_results
    hist = self.client.hub_history()
  File "<string>", line 2, in hub_history
  File "C:\data\jschulz\workspace\ipython\IPython\parallel\client\client.py", line 70, in spin_first
    return f(self, *args, **kwargs)
  File "C:\data\jschulz\workspace\ipython\IPython\parallel\client\client.py", line 1776, in hub_history
    raise self._unwrap_exception(content)
RemoteError: TypeError(can't compare datetime.datetime to NoneType)

[Carreau]

I think we haven't seen you posted a commit that fixed the test.
Pinging @minrk as he already reviewed the first part.

[jankatins]

From my understanding the error comes from something in the hub history lookup code on the hub side, not from something in the client code: the exception is also in untouched codepaths (by this patches) and before any new code is called during this test case (side effect from other test cases due to "recycling" of the hub side?)

Unfortunately I have no idea how to debug this as I can't see where this error is raised. :-( Is there any way to get at the stacktrace of the exception on the hub side?

[Carreau]

Trying to revive old PRs.

Should we do something in particular with this one ? assign someone ?

[minrk]

I'll pick up review in a week or so, just wrapping some things up right now. Assigning to myself.

[minrk]

This looks good to me, thanks for your patience @JanSchulz!
Since I can't reproduce the RemoteError you mention,
and it is in a code path you didn't touch, I don't think we need to worry about it here.
I expect I know where it is (sorting / date comparisons in the Hub, or the Hub's DB object),
so I can have a look at that myself.

@JanSchulz if you don't have anything more to add here,
I will go ahead and merge.

[jankatins]

I'm fine. Thanks for your help and comments with this PR!