Add Lwt_seq, adapted from Stdlib

[zshipko]

Adds Lwt_seq, which mirrors Seq from the standard library but the callbacks return deferred values.

Let me know if there's anything missing!

[raphael-proust]

Nice! I wanted to create something like that some time ago but I put it aside.

You could add return_nil to avoid allocations but also for completeness (see Lwt.return_nil, Lwt.return_none, etc.).

An then, the main thing that could be added is a way to map from Seq into Lwt_seq. Right now, if I'm given an 'a Stdlib.Seq.t, how do I make an 'a Lwt_seq.t? I think it'd involve a manual fold of the original seq. I'm not sure what the best interface would be for that.

Should it be a simple of_seq : 'a Stdlib,Seq.t -> 'a t?
Or maybe it should be of_seq_lwt : 'a Lwt.t Stdlib.Seq.t -> 'a t?

The first approach looks straightforward to use, but the second plays well with the lazyness of Seq.map: Seq.map files_in_a_dir read_a_file |> of_seq_lwt

Ideas? Comments?

[avsm]

Thanks @zshipko. Agree with @raphael-proust that using the pre-allocated return_nil and friends would shave off some allocations. Aside from the Seq to Lwt_seq conversions, I wonder if we need to be careful about exception propagation as well. If an exception is raised in the middle of an Lwt_seq evaluation, it should be threaded through as an Lwt exception in the return value. This will slow it down a bit vs the native one, but at least have unsurprising semantics.

[craigfe]

Suggested change

	let* seq1 = seq1 () in
	match seq1 with
	seq1 () >>= function

I think all of the uses of let* here can be simplified similarly.

[raphael-proust]

(Not that it should impact this PR, but is there a plan to have match* in OCaml soon?)

[craigfe]

Relevant PR: ocaml/ocaml#1955

[raphael-proust]

I'm ok with using the let-operators. This is mostly a matter of style. Are there any strong opinions on the matter?

I guess the main reason to use >>= instead of let* would be to keep compatibility with old ocamls, but we're already >= 4.8 which is when the binds where introduced.

[craigfe]

My suggestion for removing the let* uses in these places was because it avoids introducing a name that is immediately matched on, but I don't have a strong opinion either way :-)

[zshipko]

I just pushed some new changes with to_seq and of_seq added (in addition to fixing some of the other suggestions mentioned)

I would still like to add some tests to help demonstrate the exception situation as @avsm mentioned

[raphael-proust]

Looking into it more closely, I think that the documentation should probably mention Lwt_stream and explain the differences. Lwt_seq and Lwt_stream provide similar abstractions. Or more precisely, it is possible to use Lwt_stream carefully enough that it is indistinguishable from a Seq. Failure to acknowledge so in the doc, to point out the difference, etc. will result in confusion.

I'll draft some comment block that explains it.

[aantron]

Even though nobody will bother with this realistically, we can't have a part of the code claiming to be distributed under a (modified) LGPL license in an otherwise MIT-licensed Lwt, which doesn't (and shouldn't) make an exception for it — especially if it sends people to a LICENSE file that isn't present in Lwt.

I think the detailed authorship of this code needs to be addressed. My (uninformed) guess is that this code was originally written by @c-cube, and modified by @zshipko for Lwt. So, we should ask @c-cube for permission to distribute this variant of it in Lwt under the MIT license, in the same way as all the other Lwt code. In particular, we don't include the names of authors in each file, but rely on blame for detailed authorship history.

If @c-cube agrees, @zshipko, please change these license headers to match the ones in the rest of Lwt. If this code is indeed substantially @c-cube's, we can also express that using a Co-authored-by field in the commit, and including a note about where the code comes form in the commit's detailed message. @raphael-proust can then make sure that is preserved during the squash.

[aantron]

You can probably just write all that in the message of a future commit in this PR, probably the same one that would change the license headers, and, again, @raphael-proust can move the Co-authored-by and the history message to the right place when squashing.

[raphael-proust]

I'll make sure that the squashed commit includes the relevant info from the commit message.

[c-cube]

You can use it with the MIT license as far as I'm concerned, and there's no need to list me as an author. If you start adapting large portions of oseq (which you should 😉) you can also port it with MIT but list me as a coauthor.

[aantron]

Thanks, @c-cube!

[avsm]

If you rebase this against the master branch, the ocaml-ci should be green again.

[zshipko]

Just wanted to check in on this and see if there's anything I can do to push it along

[raphael-proust]

@zshipko I'll merge it soon (probably tomorrow). I've been a bit busy with other things so I've put the Lwt side on pause, but I'm resuming it this week.

[raphael-proust]

@zshipko I pushed two small commits on top of your branch. Please have a look and check whether they are ok. Don't hesitate to rollback some/all changes: they are mere suggestions. (Although I guess the commit for the changelog is not controversial in any way.)

The MR looks generally good. I'll just open a thread to discuss of_/to_seq which is one part I think decisions can be made.

[raphael-proust]

I'm not sure which of_seq we should have: the one currently in the MR, another, both?

Options:

• current code
  • val of_seq : 'a Seq.t -> 'a t
• Lwt-flipping
  • val of_seq : 'a Lwt.t Seq.t -> 'a t

The idea behind the Lwt-flipping of_seq is as follows.

You can already have lwt-sequence with 'a Lwt.t Seq.t. And as long as you are careful, you can consume the promises sequentially:

let rec iter_s f s =
  match s () with
  | Nil -> Lwt.return_unit
  | Cons (p, s) ->
    let* x = p in
    let* () = f x in
    iter_s f s

This works but you may accidentally step on your own feet and accidentally forget to wait for one promise to resolve before you start evaluating the next one.

The Lwt-flipping of_seq allows you to go from the valid-but-error-prone form into the Lwt-specialised version.

Maybe we need both because this Lwt-flipping of_seq is not as much as an inverse to to_seq. Maybe we can have of_seq as is, and we can have inject (or some better name) for the Lwt-flipping of_seq.

Ideas? Comments? Suggestions?

[zshipko]

The current of_seq implementation should probably stay as it is, but adding the Lwt-flipping version is a good idea.

of_seq_lwt was mentioned above - that seems a little more clear thaninject to me.

I will let you know when those changes are pushed.

[craigfe]

This function is going to force all of the sequence cells in order to run the blocking actions, which is perhaps surprising given its name. We could call it all or similar, but personally I think it's odd to support converting this structure back to a Seq.t at all. I'd be in favour of removing this function altogether.

[zshipko]

That's a good point, I'm not opposed to removing to_seq

[c-cube]

I think to_seq doesn't make much sense. Maybe there could be `to_list: 'a Lwt_seq.t -> 'a list Lwt.t`, which is clearly materializing the whole sequence; returning a "lazy" view which is actually fully materialized seems less useful imho.

[zshipko]

Replacing to_seq with to_list sounds good - I will make that update as well.

[craigfe]

(The PR already contains to_list, which I think is good 👍)

[zshipko]

Alright, just pushed a commit that removes to_seq and adds of_seq_lwt

[raphael-proust]

I added a small commit that adds a tiny bit of Lwt-specific documentation: it mentions promises and such.

I think there's room to extend the tests and to factor things out there, but as a follow-up MR rather than additional commits in this one, just to make sure it progresses.

@zshipko is it good for you? If so I'll merge and make sure there's the appropriate commit message as mentioned above.

[zshipko]

Thank you for adding that additional documentation, @raphael-proust - LGTM!