Skip to content

Add new dhall rewrite-with-schemas subcommand #1902

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

Merged
merged 26 commits into from
Jul 12, 2020
Merged

Conversation

Gabriella439
Copy link
Collaborator

@Gabriella439 Gabriella439 commented Jul 8, 2020

Fixes #1796

This adds a new dhall rewrite-with-schema subcommand that one can use to simplify
a Dhall expression using a supplied record of schemas. Any time a
subexpression matches the type of a schema it will be replaced with the
corresponding schema.

If more than one schema matches the supplied type then the first match
(in alphabetical order) is used.

Fixes #1796

This adds a new `dhall schema` subcommand that one can use to simplify
a Dhall expression using a supplied record of schemas.  Any time a
subexpression matches the type of a schema it will be replaced with the
corresponding schema.

If more than one schema matches the supplied type then the first match
(in alphabetical order) is used.
@Gabriella439
Copy link
Collaborator Author

I also tested this against dhall-kubernetes by hand (since it's too large of an example to include in our test suite). It does work (really well!), but it's slow (taking 2 minutes on my machine):

$ dhall --file ./examples/deployment.dhall | dhall schemas --record ./schemas.dhall 
let schemas = ./schemas.dhall

in  schemas.Deployment::{
    , metadata = schemas.ObjectMeta::{ name = Some "nginx" }
    , spec = Some schemas.DeploymentSpec::{
      , replicas = Some 2
      , revisionHistoryLimit = Some 10
      , selector = schemas.LabelSelector::{
        , matchLabels = Some [ { mapKey = "app", mapValue = "nginx" } ]
        }
      , strategy = Some schemas.DeploymentStrategy::{
        , rollingUpdate = Some schemas.RollingUpdateDeployment::{
          , maxSurge = Some (< Int : Natural | String : Text >.Int 5)
          , maxUnavailable = Some (< Int : Natural | String : Text >.Int 0)
          }
        , type = Some "RollingUpdate"
        }
      , template = schemas.PodTemplateSpec::{
        , metadata = schemas.ObjectMeta::{
          , labels = Some [ { mapKey = "app", mapValue = "nginx" } ]
          , name = Some "nginx"
          }
        , spec = Some schemas.PodSpec::{
          , containers =
            [ schemas.Container::{
              , image = Some "nginx:1.15.3"
              , imagePullPolicy = Some "Always"
              , name = "nginx"
              , ports = Some [ schemas.ContainerPort::{ containerPort = 80 } ]
              , resources = Some schemas.ResourceRequirements::{
                , limits = Some [ { mapKey = "cpu", mapValue = "500m" } ]
                , requests = Some [ { mapKey = "cpu", mapValue = "10m" } ]
                }
              }
            ]
          }
        }
      }
    }

I imagine that the main reason that it's slow is that it has to check every record sub-expression against every Kubernetes schema to see if they can be simplified

Copy link
Collaborator

@german1608 german1608 left a comment

Choose a reason for hiding this comment

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

Not very fluent on lenses, but the implementation looks nice!

I was thinking that maybe we could add a cli option to override the schemas binding name, in case some users prefer to use another one rather than schemas.

Althought that is not important right now.

Copy link
Collaborator

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

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

Looks neat! :)

Apologies for the many comments. Please just ignore the stupid ones! ;)

:: Expr Src Void
-> Maybe (Map Text (Expr Src Void), Map Text (Expr Src Void))
decodeSchema
(RecordLit [ ("Type", Record _Type), ("default", RecordLit _default) ]) =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh! I wasn't aware of that IsList instance! :)

It does rely on the key order BTW, which seems okay here, because the expression is normalized. Maybe worth documenting though?!

However, schemas may have more fields than just Type and default. Those wouldn't match in this case. So better use lookup!

(And I'm wondering whether it would be better to delete that IsList instance…)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That IsList instance is very convenient for nested pattern matching! It's also no different than the one for Data.Map.Map

Copy link
Collaborator

Choose a reason for hiding this comment

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

The one for Data.Map seems more predictable, since it always returns the elements in ascending key order. Maybe that's something that we should adopt…

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My reasoning for preserving the order is that the IsList instance should behave like Show, meaning that the literal it produces should be valid code for creating the original value

_ <- Core.throws (TypeCheck.typeOf resolvedExpression)

let normalizedSchemas = Normalize.normalize resolvedSchemas
let normalizedExpression = Normalize.normalize resolvedExpression
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder whether normalizing the input is such a good idea. In the kubernetes example, this expands

kubernetes.IntOrString.Int 5

to

< Int : Natural | String : Text >.Int 5

In other cases it could result in more drastic blowup.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The normalization is necessary to handle an expression like this:

{ addr.city = None Text
, addr.country = "Switzerland"
, alive = True
, name = "Alice"
}

... which desugars to:

{ addr = { city = None Text }  { country = "Switzerland" }
, alive = True
, name = "Alice"
}

If you don't normalize that then the rewrite rule won't match against the { city = None Text } ∧ { country = "Switzerland" } expression

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right. Initially I wondered whether we could not check for RecordLits and instead just check that the type of the subexpression is a record type. But that would be much more expensive. For the actual rewriting we'd also have to normalize any non-RecordLits anyway.

Another idea would be to have a similar rewrite-with-types command that could replace types with those exported from some record.

@Gabriella439
Copy link
Collaborator Author

After applying the "lookup by type hash trick" the performance for the Kubernetes example improved significantly. It now only takes 3-4 seconds to rewrite the Kubernetes example

... as suggested by @sjakobi

... and keep around `universeOf` since it can't hurt
_ <- Core.throws (TypeCheck.typeOf resolvedExpression)

let normalizedSchemas = Normalize.normalize resolvedSchemas
let normalizedExpression = Normalize.normalize resolvedExpression
Copy link
Collaborator

Choose a reason for hiding this comment

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

Right. Initially I wondered whether we could not check for RecordLits and instead just check that the type of the subexpression is a record type. But that would be much more expensive. For the actual rewriting we'd also have to normalize any non-RecordLits anyway.

Another idea would be to have a similar rewrite-with-types command that could replace types with those exported from some record.

@Gabriella439 Gabriella439 changed the title Add new dhall schema subcommand Add new dhall rewrite-with-schemas subcommand Jul 9, 2020
... as suggested by @sjakobi

This is the only thing that actually needed the `s` parameter to be
`Void`
Copy link
Collaborator

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

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

👍

To fix the AppVeyor CI, I suspect that there's a Dhall.Test.Util.toDhallPath missing somewhere, but I'm not quite sure where…

@mergify mergify bot merged commit b2f4a12 into master Jul 12, 2020
@mergify mergify bot deleted the gabriel/schema_command branch July 12, 2020 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

*-to-dhall: generate auto-completed record using a --schemas file
4 participants