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

add a sequence function based on fold_effect #52

Merged
merged 9 commits into from
Jun 2, 2015
Merged

add a sequence function based on fold_effect #52

merged 9 commits into from
Jun 2, 2015

Conversation

radix
Copy link
Contributor

@radix radix commented May 7, 2015

Since this is based on the foldE branch it has the changes from it as well. This will be remedied once foldE is merged to master. Until then I'll leave this PR in progress.

Pretty straightforward implementation of sequence based on fold_effect.

@radix radix mentioned this pull request May 7, 2015
@tomprince
Copy link
Contributor

Implementing sequence this way makes it much harder to inspect, since there are all sorts of closures involved. (See here for an example of where we do this).

def folder(acc, el):
l.append(el)
return l
return fold_effect(lambda acc, el: acc + [el], l, effects)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

folder isn't used here.

@radix
Copy link
Contributor Author

radix commented May 8, 2015

@tomprince - have you seen SequenceDispatcher? It gives you a way to test the sequence of effects in your program in a general way. Let me rewrite your example in a way that would work with this implementation (and any other that simply runs each effect in turn)

noop = lambda i: None
seq = SequenceDispatcher([
    (run(command="yum install -y %s" % ZFS_REPO[distribution]).intent, noop),
    (run(command="yum install -y %s" % CLUSTERHQ_REPO[distribution]).intent, noop),
    (run(command="yum install -y clusterhq-flocker-node").intent, noop),
])
with seq.consume():
    self.assertEqual(
        sync_perform(
            ComposedDispatcher([base_dispatcher, seq]),
            task_install_flocker(distribution=distribution)),
        [])

Granted, it's not as concise, but it has these benefits:

  • it will work with any other formulation of running some effects in sequence (like explicit callbacks)
  • It won't break if you add a pure callback to your sequence effect, or any of the run effects -- since you're comparing the Effect instances in that test, you're also comparing the effect's callbacks. If you add callbacks, you'd have to change your test to somehow avoid comparing them -- and SequenceDispatcher does this by only comparing intents, and running your callbacks (by way of performing the effect). So their behavior can be tested by either checking the return value of performing, or (if they return more Effects) inserting the appropriate intents into the SequenceDispatcher.

This kind of "I want to check equality but, oops, callbacks" was exactly one of the things that got us to switch to actually performing the effects with a testing dispatcher, and SequenceDispatcher was the best formulation of a testing dispatcher we could find, giving us the best guarantees.

So, we could add a Sequence intent, but I don't think the benefits are that great (making some testing code slightly more concise, if you're not already using SequenceDispatcher, which I think most Effect-testing code should).

@radix radix removed the in progress label May 12, 2015
@cyli
Copy link

cyli commented May 19, 2015

A couple of random musings about sequence and Sequence:

  1. I wonder of serial would make more sense, because there is already a parallel, and they're sort of related to each other (an adverb describing the way in which the effects will be run, whereas sequence is a noun). On the other hand, since flocker is already using sequence in stuff, maybe not worthwhile since they'd have to change a bunch of their code.
  2. One argument in favor of the Sequence intent would be parallel (no pun intended) construction to ParallelEffects - on the other hand, I'm not sure there's a better way to do parallel effects, because it depends on the type of async you're going to do, whereas performing things serially does not, so maybe making the two similar is pointless.
  3. If there were going to be an inspectable intent, since this is making use of fold_effects, maybe the intent, if one is desired, should be FoldEffects?

Other than that, this LGTM, and if an extra intent is required (doesn't seem like it is right now), it can probably be added in a separate PR without too much breakage. Seems pretty elegant and testable without it currently :)

@radix
Copy link
Contributor Author

radix commented Jun 2, 2015

Thanks for the comments @cyli :)

  1. I actually read sequence and parallel the same way: "run these effects in sequence" or "run these effects in parallel".
  2. yeah, you're right in that parallel is actually the special case -- parallel needs different strategies for different backends, but sequence is always the same.

I think I'll go ahead and merge this. I hope this is acceptable, @tomprince. Let me know if you want to talk more about SequenceDispatcher.

radix added a commit that referenced this pull request Jun 2, 2015
add a `sequence` function based on `fold_effect`
@radix radix merged commit 8d86762 into master Jun 2, 2015
@radix radix deleted the sequence branch June 2, 2015 16:30
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

3 participants