Add --transitive flag to dhall {format,lint,freeze}

[Gabriella439]

Fixes #1551

The dhall {format,lint,freeze} commands take a new --transitive
flag which can be used instead of --inplace to also modify any
transitive dependencies that can be reached via relative file imports.

[sjakobi]

Quite lovely!

The similarity between format, freeze and lint has me itching to extract some kind of harness that each of them could use, but that might end up creating some incomprehensible mess…

[german1608]

[style|curiosity] why don't you use _ to catch all these cases?

[Gabriella439]

It's mainly so that I get a warning if we ever extend the language to handle other import types

[german1608]

Superficially, LGTM!

Maybe @sjakobi could have another comment?

[sjakobi]

A few more optional nits. Great job, anyway! :)

[Gabriella439]

doctest mysteriously died without any error message in AppVeyor:

Test suite failure for package dhall-1.33.1
    doctest:  exited with: ExitFailure 1
Logs printed to console

I'm restarting for now to see if the problem persists

[sjakobi]

The test failure is further up in the log:

C:\\projects\\dhall-haskell\\dhall\\src\\Dhall\\Import.hs:1134: failure in expression `dependencyToFile (emptyStatus ".") Import{ importHashed = ImportHashed{ hash = Nothing, importType = Local Here (File (Directory []) "foo") }, importMode = Code }'
expected: Just "./foo"
 but got: Just ".\\foo"
Examples: 246  Tried: 243  Errors: 0  Failures: 1
dhall-1.33.1: Test suite doctest failed
dhall-1.33.1: test (suite: tasty)

Maybe that test should be turned into a simple example…

[gregziegan]

This is great! I wanted to chime in though to say that running this locally on some private libraries has resulted in >20 minute freezes. Trying to figure out what's causing the biggest shift in perf

[Gabriella439]

@thebritican: My guess is that it's most likely due to the use of dhall freeze --cache since it doubles every import it attempts to cache. This in turn leads to time taken that is exponential in the depth of the import chain

[gregziegan]

Would you say —cache is not best practice for authoring libraries? Seems necessary but I haven’t thought through the workflow implications of not using —cache

[Gabriella439]

@thebritican: One of my goals is to replace the import sha256:… ? import idiom with language support for more intelligent caching, as outlined in this comment: https://discourse.dhall-lang.org/t/figuring-out-performance-bottlenecks/251/26?u=gabriel439

It would fix several issues, including this one

[Gabriella439]

@thebritican: Also, in general you should probably not need cache for internal use cases. The original reason --cache was created was so that the Prelude would break less often when we changed the hashing algorithm by providing a fallback if the hash did not match.

[Gabriella439]

Hmmm, this branch keeps failing in AppVeyor on the doctest test suite and I can't reproduce locally

[Gabriella439]

Oh, never mind. It does log why it failed. I was just looking in the wrong place

[Gabriella439]

@sjakobi: I'm going to remove some of the doctests because they are platform-sensitive (i.e. they produce a different result on Windows)

[sjakobi]

@sjakobi: I'm going to remove some of the doctests because they are platform-sensitive (i.e. they produce a different result on Windows)

I guess the alternative would be to move them behind a bit of CPP:

#ifdef mingw32_HOST_OS
...
#endif

…although that seems a bit awkward too.

Also note that Hydra has unfortunately OOM'd in the last job…

[Gabriella439]

@sjakobi: Alright, I'll try the CPP approach

[gregziegan]

My guess is that it's most likely due to the use of dhall freeze --cache since it doubles every import it attempts to cache.

Tried this without --cache and it's definitely moving faster but still too slow for CI (30 minutes). Our bash script seems to accomplish a similar result in about 5 minutes. My next step will be researching if there's a particular package, type, or import resolution that makes this particularly slow.

[Gabriella439]

@thebritican: I think the main thing to check is a deep and linear import chain. That seems like the most likely thing to trigger an unexpected non-linear time complexity

[gregziegan]

I think the main thing to check is a deep and linear import chain. That seems like the most likely thing to trigger an unexpected non-linear time complexity

Shall I start a new issue to discuss this on this repo? Or the discourse link? It's related to import performance that this tool performs very slowly against, but it's also a more general problem.

The problem import chain:

dhall-kubernetes => module exposing opinionated constructors for dhall-kubernetes types => package => module combining opinionated constructors

Example of a very condensed and rough approximation of library structure:

-- OurK8s/Resource/Deployment.dhall
let k8s = ./dependencies/dhall-kubernetes.dhall

let standardize
  : Options {- mostly flat record w/ primitives -} -> k8s.Deployment.Type -> k8s.Deployment.Type
  = \(options : Options) -> \(deployment : k8s.Deployment.Type) ->
    withX options (withY options (withZ options deployment)))

in { standardize, withX, withY, withZ }

-- OurK8s/Resource/package.dhall
{ Deployment = ./Deployment.dhall, Service = ./Service.dhall, ... }

-- OurK8s/Role/package.dhall (a "role" combines resources often into a single document)
let k8s = ./dependencies/dhall-kubernetes.dhall

let OurResource = ./Resource/package.dhall

let role
  : MoreTuning -> k8s.Deployment.Type -> List k8s.Resource.Type
  = \(options : MoreTuning) -> \(deployment : k8s.Deployment.Type) ->
  [ k8s.Resource.Deployment (OurResource.Deployment.standardize (somethingWithOptions options)
  , k8s.Resource.Service (OurResource.Service.fromOptions options)
  , ...
  ]

There was a massive increase in performance once those two packages were removed. The import chain in other ./Kubernetes/*/package.dhall packages is more or less let k8s = ../dhall-kubernetes let otherLib = ../otherLib in k8s.Thing::{}.

$ time dhall --file ./Kubernetes/package.dhall > /dev/null # today's speed
real    0m29.908s
user    0m28.871s
sys     0m0.957s
$ time dhall --file ./Kubernetes/package.dhall > /dev/null # remove `Kubernetes.Role` from the record
real    0m4.952s
user    0m4.736s
sys     0m0.189s
$ time dhall --file ./Kubernetes/package.dhall > /dev/null # remove `Kubernetes.Role,Resource` from the record
real    0m1.163s
user    0m1.086s
sys     0m0.072s

[PierreR]

@ thebritican I am also experiencing such massive increase in performance when trying to provide a high level interface to dhall-kubernetes. I am not sure if there is a specific issue for this already (but it has been discussed in discourse).

[Gabriella439]

@thebritican: We can continue the discussion on #1890