[Go] Added input/output validation for flows and actions and model output conformance. #251
Conversation
jba
left a comment
jba
left a comment
There was a problem hiding this comment.
Thanks Alex, great stuff. Especially the extensive tests.
| return text, nil | ||
| }) | ||
|
|
||
| r := &jsonschema.Reflector{ |
There was a problem hiding this comment.
We should design an API to do this for people. Like maybe another field in dotprompt.Config that you can set to a value to reflect, instead of setting InputSchema. WDYT?
There was a problem hiding this comment.
This is a good idea. This PR is getting big so I added it to the 3 other TODOs to do in a follow up.
apascal07
changed the title
apascal07
changed the title
apascal07
changed the title
jba
left a comment
jba
left a comment
There was a problem hiding this comment.
Again, thanks for all this. Sorry it's such a beast.
And also sorry for our API updates, which will give you some merge headaches.
I would try to get this in quickly even if incomplete, to avoid future merge pain.
go/ai/document.go
Outdated
| text string // valid for kind∈{text,blob} | ||
| toolRequest *ToolRequest // valid for kind==partToolRequest | ||
| toolResponse *ToolResponse // valid for kind==partToolResponse | ||
| Kind partKind `json:"kind,omitempty"` |
There was a problem hiding this comment.
It's generally a bad idea to have an exported field of unexported type. It means users can't easily work with the field. For example, there is no way for someone to write a function that returns a value assignable to Part.Kind.
There was a problem hiding this comment.
Ah yeah, missed this. Unfortunately, had to undo all of this nice visibility control here because there's no way to generate the schema correctly any other way from what I can tell.
go/ai/generator_test.go
Outdated
| }, | ||
| } | ||
| candidate, err := validCandidate(candidate, outputSchema) | ||
| assert.NoError(t, err) |
There was a problem hiding this comment.
Case in point: what does this line actually do?
The doc for NoError says "NoError asserts that a function returned no error," but what does "assert" actually mean? I had to go and read the source code for the package.
It turns out that this line ultimately calls t.Errorf. That is not the best choice here, in my opinion. It is pointless to continue testing if the JSON isn't valid, so t.Fatalf should be called.
In this case, it doesn't matter much. But it would be a more serious bug in a case like this:
somePtrValue, err := f()
assert.NoError(t, err)
assert.Equal(somePtrValue.Field, want)
Here, somePtrValue is probably nil if err is non-nil, so the third line panics.
If you wrote out the tests and t.Error/Fatal calls explicitly, it would be more work for you, the writer, but less work for every reader to check whether your code was correct.
| return fmt.Errorf("data is not valid JSON: %w", err) | ||
| } | ||
|
|
||
| schemaLoader := gojsonschema.NewBytesLoader(schemaBytes) |
There was a problem hiding this comment.
I see, so you have to convert a jsonschema.Schema into a gojsonschema.Schema? That's unfortunate. Maybe @ianlancetaylor can help with that.
There was a problem hiding this comment.
Looking at it but this is likely the best we can do for now.
There was a problem hiding this comment.
Yeah... this is not ideal. The package you were using is the better (and much newer) of the two but it doesn't provide any validation.
No worries! Thanks for the thorough review, this guidance was very helpful. |
…ble to proceed through input validation for action -> flow.
…tput conformance. (#251)
3 participants
Summary of changes:
Partfields exported becauseinferJSONSchemawasn't generating schema for them correctly which failed validation.runJSONfor both flows and actions. This is necessary because the unmarshaling throws away unknown/invalid fields and we want to enforce clean correct inputs.Runmethod for actions andExecutemethod for flows.Generatemethods for model actions.To dos:
Config.OutputSchemaneeds to be*jsonschema.Schemainstead ofmap[string]any. Tried replacing it but it seemed to cause a panic, butConfig.InputSchemais already that type so there must be more to it...nullas an input right now; seems to convert it to an object at some point.runJSONstage display terribly (unreadably, need to look in terminal) in dev UI right now since they run outside of the tracing and dev UI only reads from tracing right now.