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

Adding Flow module #5830

Closed

Conversation

tamaskalcza-da-zz
Copy link
Contributor

@tamaskalcza-da-zz tamaskalcza-da-zz commented May 4, 2020

Pull Request Checklist

  • Read and understand the contribution guidelines
  • Include appropriate tests
  • Set a descriptive title and thorough description
  • Add a reference to the issue this PR will solve, if appropriate
  • Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
  • Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

Introducing Flow module

Inspired by F#, the pipe system in Unix and Flow module in Haskell (see: https://taylor.fausak.me/2015/04/09/write-more-understandable-haskell-with-flow/) this PR is intended to add the most basic element, the forward pipe.

How to Use

I think the code here is relatively hard to read: https://github.com/digital-asset/ex-supply-chain/blob/master/src/main/daml/DA/RefApps/SupplyChain/QuoteRequest.daml#L327-L330

Using the forward-pipe we can improve it like this:

let quotesForWarehouseSoFar =
      summedQuotes.quotesSoFar
        |> filter (\pq -> (fst pq.pricedTransportWithInventory).warehouseProduct.warehouse ==  actQuote.warehouse)
        |> map (.quantity)
        |> sum

@tamaskalcza-da-zz tamaskalcza-da-zz marked this pull request as ready for review May 5, 2020 07:33
Copy link
Contributor

@cocreature cocreature left a comment

Choose a reason for hiding this comment

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

I’m somewhat split on this. I don’t personally find myself using reverse function application very often but I don’t think that should be the deciding factor here. There are clearly a lot of people who do like this.

  • Regular function application is unavoidable (even in the test for |> you use it) so this strictly increases the set of things users need to learn. On the other hand, as long as it is in a separate module and we do not start using it heavily in things like the GSG it is mostly opt-in and users can ignore it until they want to use it.
  • I wonder if DA.Flow is really a good module name. Maybe DA.Function would be a better name? That’s where (&) is in Haskell which is the identifier for reverse function application included in base.

What do you think @hurryabit @bame-da?

@hurryabit
Copy link
Contributor

I'm split on this as well. I'm not convinced having both ($) and (|>) in the stdlib is a good idea. It pushes the burden to learn about them and how to decide when to use which on our users. Does the small benefit of not having to define it in your project outweigh that cost? I'm not sure.

Also, should we decide to use it, we need to give it a fixity that plays well with other operators, particularly $, and test that code using |> is actually interpreted the right way. If we don't add special treatment for (|>) to the compiler it might end up being significantly slower than ($). This is not trivial because the evaluation order of (|>) is not the same as the natural evaluation order in the interpreter.

@tamaskalcza-da-zz
Copy link
Contributor Author

I think reverse function application is quite natural and as you said there are lot of people who like this. "Regular" function application tends to be quite hard to read and ($) does not really help.

I find that (|>) has a very flat learning curve compared to the others. The "flow" of data, operations come naturally.

I added it to separate module, so it would be - as @cocreature said - opt-in. I'm easy on the module name. I picked Flow because of the linked Haskell library.

@shayne-fletcher
Copy link
Contributor

I would support this proposal if the function were named & and resided in DA.Function in anticipation of multi-line field projections coming via future incorporation of the upstream RecordDotSyntax.

@tamaskalcza-da-zz
Copy link
Contributor Author

I'm totally fine with putting this in DA.Function.

However, it is important that operator is |>. It builds up a computation "pipeline", hence the resambles from the Unix-pipes.

@shayne-fletcher
Copy link
Contributor

I'm totally fine with putting this in DA.Function.

However, it is important that operator is |>. It builds up a computation "pipeline", hence the resambles from the Unix-pipes.

& in Haskell means the same thing - reverse application. The symbol was perhaps poorly chosen (even SPJ has expressed regret about this) but it was so chosen for better or worse. I take the view that DAML is a Haskell derivative and as such should go with the Haskell convention to avoid impedance mismatch with general Haskell programming and documentation. Introducing OCaml conventions into the mix can only work against us IMO.

@cocreature
Copy link
Contributor

I don’t think it makes a lot of sense to base decisions on the Haskell standard library for a couple of reasons:

  1. Our primary audience is not Haskell programmers. While it is true, that advanced users will at some point look at Haskell resources to better understand DAML (simply because we cannot replicate all that for DAML specifically) getting to that point takes quite some time. I’d rather optimize for the users that are just getting started.
  2. We already deviate from the Haskell standard library in a ton of places. Action is a very prominent example of that but there are many more.
  3. Even among Haskellers, the standard library is not particularly cherished. As @shayne-fletcher mentined, a lot of people find the name of & weird. The flow library with the symbol proposed here even exists in Haskell. There are tons of alternative preludes, …

@tamaskalcza-da-zz
Copy link
Contributor Author

I agree with @cocreature. I think it's perfectly fine to be influenced by other languages if it makes things easier, helps developers.

Our primary audience is not Haskell programmers

Following this I put together a little example in various languages.

Java

List<String> inputs = Arrays.asList("1", "2", "3", "4", "5");
inputs
    .stream()
    .mapToInt(Integer::parseInt)
    .filter(x -> x % 2 == 0)
    .sum();

C#

string[] inputs = {"1", "2", "3", "4", "5"};
inputs
    .Select(int.Parse)
    .Where(x => x % 2 == 0)
    .Sum();

JavaScript

let inputs = ["1", "2", "3", "4", "5"];
inputs
  .map(x => parseInt(x))
  .filter(x => x % 2 === 0)
  .reduce((x, y) => x + y);

Scala

val inputs = List("1", "2", "3", "4", "5")
inputs
  .map(_.toInt)
  .filter(_ % 2 == 0)
  .sum

Now convert this example into DAML:

let inputs = ["1", "2", "3", "4", "5"]
    _ = sum
      $ filter (\x -> x % 2 == 0)
      $ mapOptional parseInt inputs

I think the examples above show that knowledge can be easily transferred between these languages. With DAML however, the logic is reversed and harder to follow. The proposed operator helps in this regard:

let inputs = ["1", "2", "3", "4", "5"]
    _ = inputs
      |> mapOptional parseInt
      |> filter (\x -> x % 2 == 0)
      |> sum

@garyverhaegen-da
Copy link
Contributor

garyverhaegen-da commented May 8, 2020

I'm personally quite fond of the "reverse function application" (never knew that was its name), and use it quite often, but I'm a bit skeptical of introducing a whole namespace for it. It's really easy to define as a user, and I think there is more value in telling people they can define it than in having yet another magic symbol in the standard library.

It could be a great example in the docs of how to define custom operators and what fixity means and how to choose it etc. That could be a nice "aha" moment for users in driving home the point that these operators are not built-in magic, but regular functions with unfamiliar names.

@hurryabit

This is not trivial because the evaluation order of (|>) is not the same as the natural evaluation order in the interpreter.

Isn't DAML eager? I would expect evaluation order to work fine here if we just evaluate arguments left to right before applying a function, which I always thought was the definition of eager evaluation. I suppose I need to learn more about our interpreter.

@garyverhaegen-da
Copy link
Contributor

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

@tamaskalcza-da-zz
Copy link
Contributor Author

tamaskalcza-da-zz commented May 8, 2020

"reverse function application" (never knew that was its name)

Mathematically it is reverse, as normally the notation is like f(x) and not x f.

@hurryabit
Copy link
Contributor

Isn't DAML eager? I would expect evaluation order to work fine here if we just evaluate arguments left to right before applying a function, which I always thought was the definition of eager evaluation. I suppose I need to learn more about our interpreter.

@garyverhaegen-da The problem I meant arises exactly because DAML is eager. Currently, the compiler immediately simplifies f $ x to f x, which then allows for further simplification. However, because of the evaluation order we can't simplify x |> f, which is the same as (|>) x f to f x because that would change the evaluation order. If we want to do something similar to what we do for ($) nevertheless, it would have to be let y = x in f y. That's not super complicated but easy to do wrongly.

Mathematically it is reverse, as normally the notation is like f(x) and not x f.

@tamaskalcza-da Some branches of algebra use the x f notation (and x f g for g(f(x))). I found that quite convenient. In those branches of algebra, the function composition fg also means first apply f and then g. In "standard math", the function composition f ◦ g is somehow "backward": first apply g and then f.

@tamaskalcza-da-zz
Copy link
Contributor Author

Thanks @hurryabit, that's quite interesting.

Am I getting this right that you guys find this notation convenient? If the module was renamed to DA.Function would there be any issue left? Besides fixity, on which I'm relying on you totally.

Build
It's currently failing because the test does not see the new module. How can I fix that?

@tamaskalcza-da-zz
Copy link
Contributor Author

Any news regarding this? @cocreature @hurryabit

@shayne-fletcher
Copy link
Contributor

shayne-fletcher commented May 18, 2020

Build
It's currently failing because the test does not see the new module. How can I fix that?

It seems that if you add the line import DA.Flow to compiler/damlc/daml-stdlib/src/LibraryModules.daml, the test passes.

@shayne-fletcher
Copy link
Contributor

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

@hurryabit
Copy link
Contributor

After carefully assessing the risk/reward profile of this change, we've come to the conclusion that the potential risk, as described above, does not clearly outweigh the benefit of adding |>. Since adding such a function composition operator to your project-specific prelude is a very simple endeavor, we decided to be conservative here and reject the change.
Despite rejecting this particular change, we're still very grateful every for external contributions and would like to encourage you to continue submitting changes requests. Thank you very much!

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

6 participants