Implement Async.Choice #807

eiriktsarpalis · 2015-12-17T13:47:04Z

As requested, this PR re-implements Async.Choice as presented in #744.

I've rewritten the code so that the internal async implementations are utilised, and have included a unit test that verifies the implementation using FsCheck.

msftclas · 2015-12-17T13:47:07Z

Hi @eiriktsarpalis, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

forki · 2015-12-17T13:50:21Z

Awesome! What about the tests that I added? Do you think it would make sense to include these as well?

msftclas · 2015-12-17T13:51:33Z

@eiriktsarpalis, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

eiriktsarpalis · 2015-12-17T13:53:12Z

@forki Yeah, I've seen them. The FsCheck test covers a superset of those.

forki · 2015-12-17T13:56:10Z

Cool. I closed my PR in favor of this one.
I assume you still need to merge that line from the surface area test.

eiriktsarpalis · 2015-12-17T14:01:00Z

src/fsharp/FSharp.Core/control.fs

+                match result with
+                | Choice2Of2 edi -> args.aux.econt edi
+                | Choice1Of2 [||] -> args.cont None
+                | Choice1Of2 [|P body|] -> body args


This line reflects the original fssnip implementation: if given a singleton array as argument, simply pass your current continuation to it. This does not happen in Parallel, so wondering whether it could violate expectations. @dsyme?

I think just remove this case. It's better to run all operations consistently through the thread pool.

eiriktsarpalis · 2015-12-17T14:01:54Z

@forki what line was that?

forki · 2015-12-17T14:04:42Z

https://github.com/Microsoft/visualfsharp/pull/744/files#diff-7839ba0be481ae75e07bdb3d1015eed9R576 and same for portable

eiriktsarpalis · 2015-12-17T14:06:06Z

Thanks. I completely missed that.

enricosada · 2015-12-17T14:20:54Z

src/fsharp/FSharp.Core.Unittests/FSharp.Core/Microsoft.FSharp.Control/AsyncModule.fs

+    /// Async.Choice spec is satisfied
+    let runChoice (ChoiceWorkflow(ops, cancelAfter)) =
+        // Step 1. build a choice workflow from the abstract representation
+        let completed = ref 0


we can use F# 4 i think, so let mutable completed instead of ref

I'm OK with refs being explicit if the author wishes.

@dsyme you too in the ref club with @forki , @eiriktsarpalis and @mexx ? sigh 😄

mexx · 2015-12-17T14:23:00Z

@eiriktsarpalis You also have to add the guards for the portable version in the tests, like @forki did.

Btw. IMHO the new test is less readable as the tests provided by Steffen. The main problem is that you actually check multiple properties in one test. Just my 2c.

enricosada · 2015-12-17T14:26:39Z

src/fsharp/FSharp.Core/control.fs

+
+                        let scont (result : 'T option) =
+                            match result with
+                            | Some _ when Interlocked.Increment exnCount = 1 -> 


it's safe to have an inpure function Interlocked.Increment inside when clause?
It's not possible is evalued more than once? is not better to have it after -> ?

I'd be surprised if it did.

The only possibility for this to happen would be to have the continuation fire more than once, but that's hardly related to the use of when guards per se.

I think it would be better not to do this in a when clause.

enricosada · 2015-12-17T14:30:33Z

src/fsharp/FSharp.Core.Unittests/FSharp.Core/Microsoft.FSharp.Control/AsyncModule.fs

+        let mkOp (index : int) = function
+            | NoneResultAfter t -> returnAfter t (fun () ->  None)
+            | SomeResultAfter t -> returnAfter t (fun () -> Some index)
+            | ExceptionAfter t -> returnAfter t (fun () -> raise <| ChoiceExn index)


i think <| is not used much raise (ChoiceExn index)) instead?

eiriktsarpalis · 2015-12-17T14:31:20Z

@mexx the test basically defines an algebra that models all possible inputs that could be passed to Async.Choice. The test then executes the input and verifies that it indeed is up to spec. Not really a unit test, but it certainly puts FsCheck into good use.

mexx · 2015-12-17T15:37:56Z

@eiriktsarpalis I've got the idea. FsCheck is pretty cool, I also use it a lot.
Maybe there is a way to model the different properties the implementation should hold as separate checks?
Currently there are as far as I can see six different cases that gets checked and those are the properties IMO.

isaacabraham · 2015-12-17T15:42:45Z

@mexx agree here. Ideally if that test breaks, you don't know which "property" is broken - they should ideally be one test per property so if any given test fails, you know which behaviour is playing up. You can still use FSCheck for each of those tests, of course :-)

eiriktsarpalis · 2015-12-17T15:45:15Z

@mexx I wouldn't consider this a property test, even if it does use FsCheck. It does not check against 6 separate "properties". It's merely branching out depending on the outcome of the computation. It's really just a specification test.

mexx · 2015-12-17T15:55:16Z

@eiriktsarpalis however you call it. The problem is like @isaacabraham said.
I think even the specification can be broken up into separate invariants which should hold.
The outcome is dependent on the input, right? So with properly chosen input the output should be only in one category. That should be the properties to test for.

eiriktsarpalis · 2015-12-17T19:27:24Z

@mexx I guess I'm trying to say that the assertion that this tests separate invariants is incorrect. It's really just modelling all possible inputs to the implementation, checking that the output is up to spec in each case.

dsyme · 2015-12-17T21:16:19Z

src/fsharp/FSharp.Core/control.fs

+        static member Choice(computations : Async<'T option> seq) : Async<'T option> =
+            unprotectedPrimitive(fun args ->
+                let result =
+                    try Choice1Of2 <| Seq.toArray computations


Please just use a forward pipe. We don't use much back piping in the F# compiler or FSharp.Core,

dsyme · 2015-12-17T22:15:36Z

src/fsharp/FSharp.Core/control.fsi

@@ -268,6 +268,21 @@ namespace Microsoft.FSharp.Control
        /// <returns>A computation that returns an array of values from the sequence of input computations.</returns>
        static member Parallel : computations:seq<Async<'T>> -> Async<'T[]>

+        /// <summary>Creates an asynchronous computation that executes all given asynchronous computations in parallel, 
+        /// returning the result of the first succeeding computation (i.e. the first computation with a result that is 'Some x').</summary>


This summary is a bit long. Normally it is one sentence, no i.e.

I basically just copied the format from the corresponding Async.Parallel segment.

i dont know if better, but returning blabla can be put inside returns xmldoc element

like <returns>the result of the first succeeding computation (i.e. the first computation with a result that is 'Some x'</returns>

it's used by xmldoc generator i think

mexx · 2015-12-18T10:23:45Z

src/fsharp/FSharp.Core/control.fsi

+        /// If cancelled, the computation will cancel any remaining child computations but will still wait
+        /// for the other child computations to complete.</remarks>
+        /// <param name="computations">A sequence of computations to be parallelized.</param>
+        /// <returns>A computation that returns the first succeeding computation in the sequence of input computations.</returns>


in the sequence can be misleading, as it can imply the order is preserved, what is not the case.

agreed, we need to use wording in which 'first' clearly indicates the first to complete, as opposed to the first in the sequence. Any suggestions?

KevinRansom · 2016-01-20T04:50:31Z

Thank you for this contribution.

dsyme · 2016-07-21T22:21:56Z

@eiriktsarpalis We have this test failing intermittently on windows, it may indicate a race condition:

[00:41:46] 1) Error : FSharp.Core.Unittests.FSharp_Core.Microsoft_FSharp_Control.AsyncModule.Async.Choice specification test
[00:41:46] System.Exception : Falsifiable, after 58 tests (0 shrinks) (StdGen (1910982604,296182551)):
[00:41:46] Original:
[00:41:46] ChoiceWorkflow
[00:41:46]   ([SomeResultAfter -7; SomeResultAfter 4; NoneResultAfter -1; ExceptionAfter -2;
[00:41:46]     NoneResultAfter -4; ExceptionAfter -10; ExceptionAfter 9; NoneResultAfter -9;
[00:41:46]     NoneResultAfter 10; SomeResultAfter -8],Some -14)
[00:41:46] with exception:
[00:41:46] NUnit.Framework.AssertionException:   Expected: 4000

implement Async.Choice

39527b9

msftclas added the cla-required label Dec 17, 2015

msftclas added cla-signed and removed cla-required labels Dec 17, 2015

forki mentioned this pull request Dec 17, 2015

WIP: Async.Choice #744

Closed

eiriktsarpalis reviewed Dec 17, 2015
View reviewed changes

add Async.Choice to SurfaceArea tests

c4bdb2d

enricosada reviewed Dec 17, 2015
View reviewed changes

add build conditionals for .net portable

54de8fa

enricosada reviewed Dec 17, 2015
View reviewed changes

eiriktsarpalis added 2 commits December 17, 2015 16:35

update Async.Choice description

43dd871

incorporate surface test changes

8b4cdd4

add choice to all surface area test definitions

4f44983

dsyme reviewed Dec 17, 2015
View reviewed changes

incorporate feedback

e758c51

dsyme reviewed Dec 17, 2015
View reviewed changes

fix parameter names in Async.Parallel & Async.Choice.

b81dea5

mexx reviewed Dec 18, 2015
View reviewed changes

eiriktsarpalis added 2 commits December 18, 2015 12:29

improve async.choice xml doc

28d0c0c

changes to choice xml doc

1da20e4

KevinRansom added the Approved for v.Next label Dec 21, 2015

KevinRansom merged commit 1da20e4 into dotnet:master Jan 20, 2016

eiriktsarpalis deleted the async-choice branch January 21, 2016 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Async.Choice #807

Implement Async.Choice #807

eiriktsarpalis commented Dec 17, 2015

msftclas commented Dec 17, 2015

forki commented Dec 17, 2015

msftclas commented Dec 17, 2015

eiriktsarpalis commented Dec 17, 2015

forki commented Dec 17, 2015

eiriktsarpalis Dec 17, 2015

dsyme Dec 17, 2015

eiriktsarpalis commented Dec 17, 2015

forki commented Dec 17, 2015

eiriktsarpalis commented Dec 17, 2015

enricosada Dec 17, 2015

dsyme Dec 17, 2015

enricosada Dec 18, 2015

mexx commented Dec 17, 2015

enricosada Dec 17, 2015

eiriktsarpalis Dec 17, 2015

eiriktsarpalis Dec 17, 2015

dsyme Dec 17, 2015

enricosada Dec 17, 2015

eiriktsarpalis commented Dec 17, 2015

mexx commented Dec 17, 2015

isaacabraham commented Dec 17, 2015

eiriktsarpalis commented Dec 17, 2015

mexx commented Dec 17, 2015

eiriktsarpalis commented Dec 17, 2015

dsyme Dec 17, 2015

dsyme Dec 17, 2015

eiriktsarpalis Dec 17, 2015

enricosada Dec 18, 2015

mexx Dec 18, 2015

eiriktsarpalis Dec 18, 2015

KevinRansom commented Jan 20, 2016

dsyme commented Jul 21, 2016

Implement Async.Choice #807

Implement Async.Choice #807

Conversation

eiriktsarpalis commented Dec 17, 2015

msftclas commented Dec 17, 2015

forki commented Dec 17, 2015

msftclas commented Dec 17, 2015

eiriktsarpalis commented Dec 17, 2015

forki commented Dec 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eiriktsarpalis commented Dec 17, 2015

forki commented Dec 17, 2015

eiriktsarpalis commented Dec 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mexx commented Dec 17, 2015

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

eiriktsarpalis commented Dec 17, 2015

mexx commented Dec 17, 2015

isaacabraham commented Dec 17, 2015

eiriktsarpalis commented Dec 17, 2015

mexx commented Dec 17, 2015

eiriktsarpalis commented Dec 17, 2015

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

KevinRansom commented Jan 20, 2016

dsyme commented Jul 21, 2016