stripe / skycfg

Skycfg is an extension library for the Starlark language that adds support for constructing Protocol Buffer messages.
Apache License 2.0
646 stars 54 forks source link

Non-determinism with oneofs #103

Open shubhitms opened 2 years ago

shubhitms commented 2 years ago

Here's a reproduction of the issue I'm hitting:

package main

import (
    "context"
    "fmt"

    "github.com/stripe/skycfg"
)

func main() {
    ctx := context.Background()
    config, err := skycfg.Load(ctx, "hello.sky")
    if err != nil { panic(err) }
    messages, err := config.Main(ctx)
    if err != nil { panic(err) }
    for _, msg := range messages {
        fmt.Printf("%s\n", msg.String())
    }
}
# hello.sky
pb = proto.package("google.protobuf")

def main(ctx):
  return [pb.Value(
    bool_value = True,
    string_value = "foo"
  )]

Note, Value contains a oneof - https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/struct.proto#L62 Repeatedly invocating the code now returns non-deterministic results:

$ for x in {1..5}; do ./test-skycfg; done
string_value:"foo"
bool_value:true
bool_value:true
string_value:"foo"
bool_value:true

Looks like the non-determinism is in part due to that fact that the fields of the returned proto object are stored internally in a go map[string]starlark.Value which has non-deterministic ordering, and the oneof field that is processed last wins - https://github.com/stripe/skycfg/blob/trunk/go/protomodule/protomodule_message.go#L279

Looks like the intent was to mimic the behavior in dynamicpb - https://github.com/stripe/skycfg/pull/90/files#r692217496

I decided to compare this behavior with pure python (as it is also dynamically typed and closely resembles starlark), and although you can define something similar:

> import google.protobuf.struct_pb2 as pb
> print(pb.Value(string_value="foo", bool_value=True))
bool_value: true  # deterministic

The output is deterministic (I suspect because python 3.7 introduced dictionaries that maintained insertion order https://stackoverflow.com/questions/39980323/are-dictionaries-ordered-in-python-3-6)

Are the authors open to adding determinism here? This can result in hard to reason about configurations due to inadvertent user error. One naive solution would be to sort fields by name to ensure some type of deterministic ordering.