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

Non-blocking periodic_events method on DynamicMap #1429

Merged
merged 18 commits into from May 16, 2017

Conversation

Projects
None yet
3 participants
@jlstevens
Member

jlstevens commented May 10, 2017

This PR aims to address #1428 and replaces the Periodic stream prototype we were testing. It should be possible for the bokeh renderer to install a bokeh appropriate utility when running as a bokeh app.

Action items

  • Add general periodic utility to core (simple threaded approach)
  • Add periodic_events method to DynamicMap.
  • All count of None to run indefinitely
  • Bokeh server support (i.e when running as a bokeh app)
  • Update tutorials to mention this and use it
  • Unit tests

@jlstevens jlstevens added this to the v1.7.1 milestone May 10, 2017

@jlstevens

This comment has been minimized.

Member

jlstevens commented May 10, 2017

@philippjfr

I don't have anything more to do on this PR right now. If you could figure out how to get this working when running as a bokeh app, that would be great!

I won't be touching this again (until you tell me to) so don't worry about merge conflicts and just push your changes here. Meanwhile I'll prepare some unit tests for the default util but I won't commit that yet.

@jlstevens

This comment has been minimized.

Member

jlstevens commented May 10, 2017

Maybe a stop() method on DynamicMap would be nice instead of having to grab a handle on something that has stop(). DynamicMap would have to hold onto these thread like objects internally so it could stop all instances that might still be running.

@jlstevens

This comment has been minimized.

Member

jlstevens commented May 14, 2017

In the last commit, I implemented a slightly different approach - a periodic object with a stop method. This means you can do something lik this:

dmap.periodic(0.001, 1000000)
dmap.periodic.stop() # To stop whenever you want 

In addition to making stopping periodic callbacks more convenient, this approach allows _periodic_util attribute to be set on the periodic class. This can be set to something different from the default e.g in a bokeh app context. In addition the instances of this utility are available via the instances attribute which, by default, is used to stop anything that is running but can also be accessed by the renderer if necessary.

@philippjfr I think this approach should fit better with the bokeh app support. Over to you!

@jlstevens

This comment has been minimized.

Member

jlstevens commented May 14, 2017

I've just had another thought: maybe it would be conceptually simpler to only have one instance.

If we did that, then when you run __call__, it stops that instance (if it is already running). As the event method can update all the stream arguments at once, I'm not sure why we would want to have multiple periodic runner instances.

if isinstance(o, DynamicMap):
layer_streams += get_nested_streams(o)
return list(set(layer_streams))
return list({s for dmap in get_nested_dmaps(dmap) for s in dmap.streams})

This comment has been minimized.

@jlstevens

jlstevens May 15, 2017

Member

Makes sense to refactor it this way.

else:
plot.document = doc
plot = plot.plot
plot.document = doc

This comment has been minimized.

@jlstevens

jlstevens May 15, 2017

Member

Maybe add a comment as a reminder of what attach_periodic does here...

@classmethod
def is_set(cls):
return self.count == 0
return completed

This comment has been minimized.

@jlstevens

jlstevens May 15, 2017

Member

I'll update the API of the other periodic utility so it doesn't need to be this complex. I'll also simplify this.

@jlstevens

This comment has been minimized.

Member

jlstevens commented May 15, 2017

@philippjfr All looks good!

I'll update the stuff around completed and is_set to simplify it, but otherwise we just need to add unit tests and update the docs. Maybe one of the existing bokeh app examples should use it...if not maybe add another one?

Any way you can set up bokeh specific unit tests for that utility? I'll add some unit tests for the core utility...

def __str__(self):
return "<holoviews.core.spaces.periodic method>"

This comment has been minimized.

@jbednar

jbednar May 15, 2017

Member

Maybe add a comment explaining what this representation is meant to achieve?

This comment has been minimized.

@jlstevens

jlstevens May 15, 2017

Member

If you have another idea of what the repr should be, I would be happy to implement it! What sort of comment are you thinking of?

This comment has been minimized.

@jbednar

jbednar May 16, 2017

Member

Just something to indicate why the default representation was not suitable. I.e. why was this method necessary?

This comment has been minimized.

@jlstevens

jlstevens May 16, 2017

Member

It isn't strictly necessary, but I think it is a bit nicer than the default which is something like:

<holoviews.core.spaces.periodic at 0x10886e850>

Instead of a random memory address, I say that it is a method which reflects how this utility is used (i.e it lives on DynamicMap).

Happy to remove it if you think the default is better.

This comment has been minimized.

@jbednar

jbednar May 16, 2017

Member

No need to remove it, just put a docstring saying what you say here (that this repr provides additional information that should be more useful).

@jlstevens

This comment has been minimized.

Member

jlstevens commented May 16, 2017

@philippjfr Ready for review once the tests go green.

@jlstevens

This comment has been minimized.

Member

jlstevens commented May 16, 2017

@philippjfr The pr tests have passed and I don't think the push build ones will unless I rebase.

@philippjfr

This comment has been minimized.

Member

philippjfr commented May 16, 2017

Looks good, merging.

@philippjfr philippjfr merged commit 8b68e26 into master May 16, 2017

3 of 4 checks passed

continuous-integration/travis-ci/push The Travis CI build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.1%) to 78.739%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the periodic_utility branch May 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment