v1.1 -> v2.0 Semantics refactor #80

ejhumphrey · 2017-04-09T17:38:16Z

Renames tests for clearer mappings between submodules
Addresses much of the conversation from #75

will break the current API, and needs some help signaling deprecations
Streamer.generate is now Streamer.iterate, since it returns an iterator
Streamertypes are now directly iterable
Mux semantics are simplified to remove the (now superfluous) seed concept, which were synonymous with streams; now there are only streamers and streams.
the notion of batch is scrubbed in all core functionality.

Will resolve: #64, #75, #76, #77, #78, #79, #81

…cism

bmcfee

Changes mostly look good. There are a couple of things I'd like to see added, and we need to properly handle deprecation and parameter renames.

bmcfee · 2017-04-09T17:42:13Z

pescador/buffered.py

@@ -56,31 +58,31 @@ def activate(self):
        """Activates the stream."""
        self.stream_ = self.streamer

-    def generate(self, max_batches=None):
+    def iterate(self, max_iter=None):


persist the old generate function here. we can wrap the call signature for API compatibility, but there should be a warning.

@bmcfee do you have an example of "@Move"ing a class's method? I could hack it out of if need be, but I'd rather do the fast thing.

bmcfee · 2017-04-09T17:43:55Z

pescador/buffered.py

-def buffer_batch(generator, buffer_size):
-    '''Buffer an iterable of batches into larger (or smaller) batches
+def buffer_batch(streamer, buffer_size):
+    '''Buffer data samples from an streamer into one data object.


this doesn't actually require a streamer, right?

no, it needs an iterable; not sure what a sane variable name would be here. I also aspire to change this on a subsequent PR, so .... any strong opinions?

I've opted for stream

bmcfee · 2017-04-09T17:44:32Z

pescador/core.py

@@ -8,6 +8,11 @@

 class StreamActivator(object):
    def __init__(self, streamer):
+        for mname in ['activate', 'deactivate']:


why not just check isinstance(streamer, Streamer) instead?

tl;dr: ya, that's better.

commentary: I think I had a lapse of Java ptsd, and wanted to check that the object implements the interface of activate/deactivate, since it doesn't strictly matter that it's a Streamer (although when would we run into that? /shrug). If we really cared, a more Pythonic way of doing this would be to define an ActiveStream mixin, but ... this is total overkill for where we are right now, and mention it now only for completeness.

bmcfee · 2017-04-09T17:45:36Z

pescador/core.py

-            raise PescadorError('streamer must be a generator, iterable, or '
-                                'Streamer')
+            raise PescadorError('`streamer` must be a callable function that '
+                                'returns an iterable object.')


could we actually generalize this and support iterables as well as functions? Call it a stretch goal.

Actually it does as of this change 😄 ... I've updated the error message, and added a Streamer(iterable) test in da42dbf.

bmcfee · 2017-04-09T17:46:49Z

pescador/core.py


    def deactivate(self):
        self.stream_ = None

-    def generate(self, max_batches=None):
-        '''Instantiate the generator
+    def iterate(self, max_iter=None):


again, deprecation rename / persist old signature with warning

roger roger

bmcfee · 2017-04-09T17:49:43Z

pescador/core.py


+    # TODO: ejhumphrey would like to deprecate this class method and turn it
+    #       into a "transform". @bmcfee please advise? example to follow?
    def tuples(self, *items, **kwargs):
        '''Generate data in tuple-form instead of dicts.



I'm still not 100% on what a transform is defined to be.

I find the .tuples() interface quite useful for interfacing with keras though, and I'd prefer to not have to replace it with something more cumbersome.

I've added a long-form response here, including why I think this is an anti-pattern.

tl;dr: Streamer.tuples only works for a subset of data streams that might be fed through it; I'd propose explicitly separating Streamer functionality from processing data streams.

The thing I'd like to replace it with is a separate tuples function (as sketched out in that linked issue), which I'd argue isn't any more cumbersome than what we currently have and Streamer remains data stream agnostic.

bmcfee · 2017-04-09T17:51:04Z

pescador/mux.py

-                 lam=256.0, pool_weights=None, with_replacement=True,
-                 prune_empty_seeds=True, revive=False,
+    def __init__(self, streamers, k,
+                 lam=256.0, weights=None, with_replacement=True,


deprecation renames EVERYWHERE

while we're at it though, I'd like to replace lam=>rate

bmcfee · 2017-04-09T17:52:01Z

pescador/zmq_stream.py

@@ -81,15 +81,16 @@ def zmq_recv_batch(socket, flags=0, copy=True, track=False):
    return results


-def zmq_worker(port, streamer, terminate, copy=False, max_batches=None):
+def zmq_worker(port, streamer, terminate, copy=False, max_iter=None):


bmcfee · 2017-04-09T17:59:15Z

@ejhumphrey Re: handling variable rename deprecations, here's how librosa does it. Note that the current stable version has no lingering renames, but rolling back to 0.4.3 you can find some examples.

Check out the util.deprecation submodule
Here's an example of it in use: https://github.com/librosa/librosa/blob/446708889bf04438aaac7fe64aa738a09955bbf6/librosa/core/constantq.py#L327-L329
There's also some helpers in the util.decorators submodule for deprecating entire functions, or moving them to different submodules/renaming them.
Here's an example of move/rename behavior: https://github.com/librosa/librosa/blob/446708889bf04438aaac7fe64aa738a09955bbf6/librosa/core/audio.py#L692-L693
Here's an example of deprecated function behavior: https://github.com/librosa/librosa/blob/446708889bf04438aaac7fe64aa738a09955bbf6/librosa/feature/spectral.py#L1337-L1338

The futurepast package implements some of these things and more, but it's not yet at an initial release.

ejhumphrey · 2017-04-09T19:59:33Z

thanks @bmcfee, made it pretty far ... still have some work to do, but any advice / pointers will be helpful.

bmcfee · 2017-04-09T20:55:57Z

@ejhumphrey I don't have an example of @moveing a class method, because that gets a bit more involved. However, I think you don't necessarily want or need that here, since the call signature has changed as well. I would just write a shim function to translate to the new API, and a custom warning message.

ejhumphrey · 2017-04-14T22:12:35Z

alrighty, updated the examples, and added some slightly more meaningful benchmarking to the zmq_example, which burns cycles to prove that things are happening in the background.

LET'S DO THIS

bmcfee · 2017-04-14T22:47:04Z

examples/frameworks/keras_example.py

    X = np.atleast_2d(X)
-    # y's are binary vectors, and should be of shape (1, 10) after this.
+    # y's are binary vectors, and should be of shape (10,) after this.


atleast_2d will make them shape (1,10)

Agreed; get rid of the atleast_2d on the Y, if that' swhat you intend (which I think it is).

bmcfee · 2017-04-14T22:48:15Z

examples/mux_files_example.py

@@ -61,14 +61,14 @@ def npz_generator(npz_path):
    """Generate data from an npz file."""
    npz_data = np.load(npz_path)
    X = npz_data['X']
-    # y's are binary vectors, and should be of shape (1, 10) after this.
+    # y's are binary vectors, and should be of shape (10,) after this.


same comment here, i think?

The comment about the shape is almost certainly not right here.

bmcfee · 2017-04-14T22:54:28Z

pescador/maps.py

+        inputs = [inputs]
+    if outputs and isinstance(outputs, six.string_types):
+        outputs = [outputs]
+


Upon closer inspection, I think this still isn't correct.

I don't think fit(inputs=x) and fit(inputs=[x]) do the same thing (I might be wrong on that though). If that's the case, then upcast-downcast will not behave as expected.

cjacoby

LGTM, with some minor nits, specifically:

version increment in separate PR?
Don't forget about documentation updates! (/doc/...). I could help with that though, if you like. I'd also be for that being in another PR.

cjacoby · 2017-04-14T23:44:32Z

examples/frameworks/keras_example.py

    X = np.atleast_2d(X)
-    # y's are binary vectors, and should be of shape (1, 10) after this.
+    # y's are binary vectors, and should be of shape (10,) after this.


Agreed; get rid of the atleast_2d on the Y, if that' swhat you intend (which I think it is).

cjacoby · 2017-04-14T23:49:24Z

examples/mux_files_example.py

@@ -61,14 +61,14 @@ def npz_generator(npz_path):
    """Generate data from an npz file."""
    npz_data = np.load(npz_path)
    X = npz_data['X']
-    # y's are binary vectors, and should be of shape (1, 10) after this.
+    # y's are binary vectors, and should be of shape (10,) after this.


The comment about the shape is almost certainly not right here.

cjacoby · 2017-04-14T23:56:31Z

pescador/buffered.py

            number of *buffered* batches that are generated,
            not the number of individual samples.
+
+        partial : bool, default=True
+            Return buffers smaller than the requested size.


I prefer the phrasing in the other case this came up, below; Return buffers smaller than the requested size. makes it sound like it always does that, which is not quite what it means. (But, a 'nit really)

cjacoby · 2017-04-15T00:04:23Z

pescador/mux.py

+            Use the `streamers` parameter instead.
+            The `seed_pool` parameter will be removed in pescador 2.0.
+
+        lam : float > 0.0


💯 on converting lam to rate - I think that's sooo much more intuitive.

However, you missed including rate in the docs here. I suspect the lam above is supposed to be rate.

yup, caught it

cjacoby · 2017-04-15T00:07:55Z

pescador/version.py

@@ -3,4 +3,4 @@
 """Version info"""

 short_version = '1.0'
-version = '1.0.0'
+version = '1.1.0'


@bmcfee - Correct me if I misunderstand the case, but I believe last time I tried to do this that you requested all version increments be in a separate PR. I would expect that to apply here.

Also... shouldn't we increment the short_version too?

can I trouble y'all for consensus? I'd love to get this over the goal line today 😄

Plz see: https://github.com/pescadores/pescador/blob/master/CONTRIBUTING.md, "Additional Comments".

roger roger

cjacoby · 2017-04-15T00:09:20Z

tests/test_buffered.py

+
+    T.__eq_lists(reference, estimate)
+
+    estimate = pescador.BufferedStreamer(gen_stream, buf_size)


curious; is this copypasta, or have you kept the old interface here on purpose? Same below.

(If these tests are here to make sure the old interface still works, that's fine, just mention that in a comment someplace, and that they should be removed when the deprecations are removed.)

ya, the whole buffered submodule is going away in 2.0, I've added a comment.

ejhumphrey · 2017-04-17T15:30:02Z

couple comments here because this thread is getting insane.

versioning: what's our stance (this vs separate)?
docs: help would be great. also I dunno what I'm doing. so help would be really great.

cjacoby · 2017-04-17T16:27:25Z

Re: Versioning. I'm posting this here, too 'cause embedded is getting too crazy.

The CONTRIBUTING.md says:
Please keep branches as simple as possible: do not implement multiple features in the same branch, unless they are mutually dependent on one another. By the same token, please do not change unrelated portions of the repository (e.g., version numbers or changelog entries).
and from that, I'd say, separate PR for version update.

Re: Docs - let's start an Issue with a checklist?

cjacoby · 2017-04-17T16:54:26Z

(Once we have those two things, I think this PR is 👍 )

ejhumphrey · 2017-04-17T17:08:47Z

hey team, two things:

for clarity, you're saying create a docs issue and fix it in this PR? or create an issue and fix it elsewhere?
in parallel, I started updating a recent project to (what will be) pescador 1.1, and having some difficulties dealing with Mux, specifically, the Mux-of-Muxes case that remains a little bit suspicious, as far as I'm concerned. I'm still in the process of determining whether my current struggles are a result of poor initial design of data streams in that project, a fundamental issue with the Mux API, or some combination of both. Regardless, I don't want to merge this PR until I've got concrete answers about what the hell is going on.

cjacoby · 2017-04-17T18:54:54Z

Re: docs. I'm proposing it gets fixed in a separate PR. The items could get logged in that PR, or in an issue. But I'd start with an issue at least. If you write up an issue with all the components that changed such that they will affect the docs, I could take a stab at updating the docs to reflect the changes made in a PR.
Re: Muxes. This sounds like a good candidate for identifying via improved testing, ya?

ejhumphrey · 2017-04-17T18:58:56Z

pescador/mux.py

-            If ``True``, Streamers that previously produced no data are never
+        prune_empty_streams : bool
+            Disable streamers that produce no data.
+            If ``True``, streamers that previously produced no data are never
            revisited.
            Note that this may be undesireable for streams where past emptiness
            may not imply future emptiness.

        revive: bool
            If ``with_replacement`` is ``False``, setting ``revive=True``


@bmcfee @cjacoby, pop quiz ... if with_replacement=True, what is the significance of revive?

…ile.

ejhumphrey · 2017-04-18T16:04:03Z

@bmcfee, can I trouble you for a ✅ ?

bmcfee · 2017-04-18T19:41:54Z

pescador/mux.py

                break

+            # TODO: Stream uniqueness can't be guaranteed here.
+            # Other machinery necessary to impose permutation, this is
+            # implicitly `with_replacement`


really? I thought we zero out the probability of active streams when with_replacement=False.

You're correct, and I've already removed the comment locally; this was my read at midnight last night, and fixed it this am with some coffee-inspired clarity.

btw I was totally wrong and just thought i did (uhderp)

bmcfee

LGTM. One dangling rejoinder to a todo/comment you left in mux, but otherwise fine.

ejhumphrey · 2017-04-18T21:31:44Z

replaced by #88

Eric Humphrey added 10 commits April 3, 2017 19:22

Renamed tests for consistency with submodules.

f5fa17c

Added a StreamActivator test + refactored for single submodule.

c1d3aa0

First pass at linguistic polish; adds __iter__ iterface to Streamer

06bdaf5

Semantic tweaks to mux

872ae1a

Semantic mods to buffered + zmq

7e1cc33

Fixed semantic confusion after brushing up on terminology.

faf7c87

Scrubbed Streamer semantics, s/generate/iterate

05bb1bd

Scrubbed buffer semantics.

97ce093

Scrubbed Mux semantics.

71be519

Cleaned up ZMQStreamer, minified tests for Streamer iteration agnosti…

44d82c2

…cism

ejhumphrey requested review from cjacoby and bmcfee April 9, 2017 17:38

bmcfee requested changes Apr 9, 2017

View reviewed changes

bmcfee added the API label Apr 9, 2017

bmcfee modified the milestones: 1.1.0, 2.0.0 Apr 9, 2017

bmcfee assigned ejhumphrey, cjacoby and bmcfee Apr 9, 2017

Cleaned iterate calls in test_core, added Streamer(iterable) test.

da42dbf

ejhumphrey mentioned this pull request Apr 9, 2017

Composing generators: good idea or great idea? #77

Closed

Dropped a bunch of deprecation breadcrumbs all over the place.

179d2d1

Eric Humphrey added 4 commits April 10, 2017 17:21

Added decorator dependency to setup.py

37674ad

Added deprecation hooks to core

bdbf0a1

Introduces pescador.maps with buffer_stream test.

265dee7

Moves batch_length and test to util

0a6eca0

Eric Humphrey added 3 commits April 14, 2017 16:56

Fixed up mux_files example

202fda7

Reworked zmq example for burning cycles, demonstrates backgrounding.

5fdb323

Updated notebook

eb91dba

bmcfee reviewed Apr 14, 2017

View reviewed changes

bmcfee requested changes Apr 14, 2017

View reviewed changes

cjacoby requested changes Apr 15, 2017

View reviewed changes

Incorporated bmcfee + cjacoby feedback.

b65d43f

ejhumphrey commented Apr 17, 2017

View reviewed changes

Eric Humphrey added 2 commits April 17, 2017 15:18

Added critical mux test with streamer interruption.

893c7ed

Reverted version numbering, added mux comments

58da35c

cjacoby approved these changes Apr 18, 2017

View reviewed changes

Eric Humphrey added 2 commits April 18, 2017 10:06

Added test for mux reboot failure

055e4ac

Added simpler test of re-mux failure

ce75025

ejhumphrey mentioned this pull request Apr 18, 2017

Unable to re-iterate Muxes #86

Closed

Added inf-loop xfail test and pytest-timeout to test-optional setup f…

adf037e

…ile.

bmcfee reviewed Apr 18, 2017

View reviewed changes

bmcfee approved these changes Apr 18, 2017

View reviewed changes

final nits

fda7b0f

ejhumphrey mentioned this pull request Apr 18, 2017

Refactor of semantics and tests en route to 2.0 #88

Merged

ejhumphrey closed this Apr 18, 2017

bmcfee mentioned this pull request Dec 8, 2017

Mux Refactor #103

Merged

4 tasks


		T.__eq_lists(reference, estimate)

		estimate = pescador.BufferedStreamer(gen_stream, buf_size)

v1.1 -> v2.0 Semantics refactor #80

v1.1 -> v2.0 Semantics refactor #80

Conversation

ejhumphrey commented Apr 9, 2017 • edited Loading

bmcfee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmcfee commented Apr 9, 2017

ejhumphrey commented Apr 9, 2017

bmcfee commented Apr 9, 2017

ejhumphrey commented Apr 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjacoby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejhumphrey commented Apr 17, 2017

cjacoby commented Apr 17, 2017 • edited Loading

cjacoby commented Apr 17, 2017 • edited Loading

ejhumphrey commented Apr 17, 2017

cjacoby commented Apr 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejhumphrey commented Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmcfee left a comment

Choose a reason for hiding this comment

ejhumphrey commented Apr 18, 2017

ejhumphrey commented Apr 9, 2017 •

edited

Loading

cjacoby commented Apr 17, 2017 •

edited

Loading

cjacoby commented Apr 17, 2017 •

edited

Loading

ejhumphrey commented Apr 18, 2017 •

edited

Loading