Add Transact*Task api's to the F# decider abstraction #375

nordfjord · 2023-04-04T17:18:11Z

We're moving towards using Tasks more and more and this API would be helpful in situations such as calling deciders from propulsion as propulsion handlers are now Task based while deciders are still Async.

bartelink · 2023-04-05T08:09:33Z

src/Equinox/Decider.fs

+    /// 2. (if events yielded) Attempt to sync the yielded events to the stream.
+    ///    (Restarts up to <c>maxAttempts</c> times with updated state per attempt, throwing <c>MaxResyncsExhaustedException</c> on failure of final attempt.)
+    /// 3. Uses <c>render</c> to generate a 'view from the persisted final state
+    member _.TransactTask(interpret : 'state -> CancellationToken -> Task<'event list>, render : 'state -> 'view, ?load, ?attempts, ?ct) : Task<'view> =  task {


Have you experimented with other signatures? e.g. would calling it TransactAsync work or does it mess up intellisense? (I can see the reasoning about giving specific names - we can always add aliases later but you can't undo an overloaded name as easily)
is the list -> seq hop still valuable (IIRC in the async case, it mattered somewhere at some time, but that might have been an old compiler/VS toolset on net45 a long time ago)

Also wondering whether exposing a lower level TransactExAsync (or a Property exposing inner) might allow this overall need to be served by adding extension methods/type augmentations locally vs requiring every impl to be a PR and release?

Also wondering are any of these signatures useful in dotnet-templates?

Not sure if it counts but there are a fair few Propulsion.Internal.Async.startImmediateAsTask in dotnet-templates that I believe could be (with some additional changes) removed with this change

https://github.com/search?q=repo%3Ajet%2Fdotnet-templates%20Propulsion.Internal.Async.startImmediateAsTask&type=code

Have you experimented with other signatures?

I'll admit that I didn't, I've been knee deep in OCaml (which doesn't support any overloading whatsoever) for the past few days so it didn't even occur to me to overload.

If it is possible would that be your preference?

is the list -> seq hop still valuable

I'm not sure to be honest, I assumed it was there intentionally.

The root issue here is that we need to know if there are events pretty much straight away in https://github.com/jet/equinox/blob/master/src/Equinox/Core.fs#L44
That was once boring F# async code, and list made sense - pattern matching etc
Recently for C#/DeciderCore, I made that be Task<_, 'res * 'e seq> as a LCD between C# yield returns (which IIRC can't be used to build arrays neatly?) and the lists from F#
I suspect that changing the list to seq will trigger some compiler errors in F# consumption in some relatively exotic cases. (I think I validated that, but not 100% sure)

Isn't a list already a seq? can't we just pass the list?

Yes, it is. There are exotic cases (e.g. >> etc) wherein things sometimes break down though. They are definitely pretty rare and/or arguably shortcomings in type inference and/or type generalization

bartelink · 2023-04-05T08:10:42Z

src/Equinox/Decider.fs

+    /// 2. (if events yielded) Attempt to sync the yielded events to the stream.
+    ///    (Restarts up to <c>maxAttempts</c> times with updated state per attempt, throwing <c>MaxResyncsExhaustedException</c> on failure of final attempt.)
+    /// 3. Yield result
+    member _.TransactTask(decide : 'state -> CancellationToken -> Task<'result * 'event list>, ?load, ?attempts, ?ct) : Task<'result> =  task {


what would you think about forcing decide to return struct tuples? and maybe seq ?

decide to return struct tuples?

I'd love it if F# removed the need to add the struct keyword from the calling side (does it do that yet?). I've noticed the struct tuple / tuple divide cause confusion in my team at least. Though yes, I do agree that a struct tuple is appropriate here. I'm torn 🤔

and maybe seq?

I think that'd go against the programming model. We encourage users to set up deciders in a certain way that uses lists, and as such I don't think moving the seq transformation to the client provides a lot of value.

In general, decide functions don't get explicit type specs in source code (and I'm absolutely not adding type aliases!)
This means that the typical code on the client side will generate obj tuples unless people go typing struct

Having said that, returning a 'result and an event list is not the most common thing so other considerations can drive it

It's important to me that this final V4 API not look stupid in the context of normal F# code in 3 years time, even if that makes it quirky in the short term. I am unaware of (and don't expect) any incoming way to flip to struct tuples as a default (some funny ,, operator?!) though, which makes me think it needs to remain obj tuples.

My thinking was to leave the 'idiomatic' interface using Async+obj tuples+FSharp list due to a) it being idiomatic F# b) perf not mattering coz IO c) wanting continuation token propagation

If the CT is being passed, I guess requiring a struct tuple for the result is not a lot to ask though (esp if the outer ct becomes mandatory?)

bartelink · 2023-04-05T08:14:52Z

src/Equinox/Decider.fs

+    ///    (Restarts up to <c>maxAttempts</c> times with updated state per attempt, throwing <c>MaxResyncsExhaustedException</c> on failure of final attempt.)
+    /// 3. Yields a final 'view produced by <c>mapResult</c> from the <c>'result</c> and/or the final persisted <c>ISyncContext</c>
+    member _.TransactExTask(decide : ISyncContext<'state> -> CancellationToken -> Task<'result * 'event list>, mapResult : 'result -> ISyncContext<'state> -> 'view,
+                             ?load, ?attempts, ?ct) : Task<'view> =  task {


remove 2 spaces ;)

😂, even my fancy vim indent config can't handle your demands!

there's a pile of = sequences [already in there], can you clean them too while you're a it please?
Also for avoidance of doubt, the ?load being out of alignment is the reason I commented here

bartelink · 2023-04-05T08:34:51Z

The Task vs Async naming feels a bit wonky on first look, even though it's following the logic of the naming in general. Other questions spring to mind:

should ct args be mandatory ? In general the reason I didn't just expose both on a single abstraction is that I wanted people to think before giving up the flowing of cancellation - in general given a Transact will involve an IO roundtrip, the perf overhead of Async vs Task is not going to be significant.
is forcing list and using non-struct tuples still appropriate if going task?
is this best addressed by exposing a single low level method and/or exposing inner as an Inner/Coreproperty and then letting people build extension methods? (I'm open to pushback that all you're doing is adding equivalentTaskAPIs for anyAsync` ones, but part of me feels we should validate that need in multiple systems before baking it). (In general I'd aim to keep DeciderCore minimal, i.e. no FSharp* signatures, but maybe some noise in there might be worthwhile if it keeps the F# layer easier to read through top to bottom?)

I do appreciate the overall idea of removing technical noise from business logic of course, but perhaps having some helpers in a ubiquitous lower level shims lib might address that more generally e.g. fsprojects/FSharp.Control.TaskSeq#142

bartelink · 2023-04-14T06:54:31Z

With the latest changes, I wonder whether doing a member val Core = inner would not give you pretty much the same experience? or you could call it Task or Internal. Either way, providing that would give an escape hatch for concocting arbitrary variants by calling TransactExAsync on that?

Are you using something like IcedTasks for CancellationToken -> Task<'T>? If not, would skipping passing the CT match the real world usage in your app? Obviously one would not expose that in the real API, but having customised helpers live as type augmentations in a local helper lib might be a route to customising that (and hiding threading of cancellation)

bartelink · 2023-04-17T11:37:25Z

We're moving towards using Tasks more and more and this API would be helpful in situations such as calling deciders from propulsion as propulsion handlers are now Task based while deciders are still Async.

The next RC of Propulsion will provide a lower level API based on Tasks, Funcs and struct tuples
However the default API (without Async suffices in Factory) will continue to use Async, obj tuples, and FSharpFuncs
Having _ct and/or Async.AwaitTask in prod code can wait until a profiler tells you which ones need it

For this release, I'm going to expose a val Core : DeciderCore so you can further prototype to cater for such needs by having type augmentations on Decider in a global namespace within an app.

As discussed out of band, the key issue with the APIs in DeciderCore from a F# perspective is that they operate on Event seq rather than Event list (that and the struct tuples)

I'm hopeful that taking a bit longer before baking something noisy in will ultimately yield a better overall result, even if it's not entirely clear to either of us what that might actually become.

nordfjord added 2 commits April 4, 2023 13:17

feat: Add Decider.TransactTask apis

3f2cbcd

fix: comments

dd94a6c

bartelink reviewed Apr 5, 2023

View reviewed changes

bartelink mentioned this pull request Apr 13, 2023

DynamoDB: EventContext fails when tip not read #377

Merged

just use an async overload and require the user to pass in a seq

ea3e673

bartelink mentioned this pull request Apr 14, 2023

Add Sinks Factory, Batched mode jet/propulsion#208

Merged

bartelink closed this Apr 17, 2023

Add Transact*Task api's to the F# decider abstraction #375

Add Transact*Task api's to the F# decider abstraction #375

Uh oh!

Conversation

nordfjord commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bartelink Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bartelink Apr 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bartelink commented Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bartelink commented Apr 14, 2023

Uh oh!

bartelink commented Apr 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nordfjord commented Apr 4, 2023 •

edited

Loading

bartelink Apr 5, 2023 •

edited

Loading

bartelink Apr 14, 2023 •

edited

Loading

bartelink commented Apr 5, 2023 •

edited

Loading