[BEAM-6825] Improve Combine error messages. #8243
Conversation
|
R: @youngoli Please review and ask all the questions. |
youngoli
left a comment
youngoli
left a comment
There was a problem hiding this comment.
Looks good, only a few comments for readability improvements.
sdks/go/pkg/beam/core/graph/fn.go
Outdated
| e.fnKind, e.methodName, name, typ, err.Got, e.accumType, e.sig) | ||
| } | ||
| } | ||
| return fmt.Sprintf("%s invalid %v %v: \"%v\" for %v; expected a signature like %v", |
There was a problem hiding this comment.
This error is much harder for me to parse than the other ones in this method. I think it could be improved by a few more descriptive words and structuring it more like other errors in this codebase, where the original error is appended to the end. Here's an example I think works better:
{e.fnKind} invalid {e.methodName} {name}: got type {typ} but expected a signature like {e.sig}, original error: {e.err}
| // ExtractOutput func(A) (O, error?) | ||
| // This means that the other signatures *must* match the type used in MergeAccumulators. | ||
| if len(mergeFn.Ret) <= 0 { | ||
| return nil, fmt.Errorf("%v: %v requires at least 1 return value. : %v", fnKind, mergeAccumulatorsName, mergeFn) |
There was a problem hiding this comment.
This whole section below is really repetitive, with a few small differences in the calls to funcx.Replace. Finding a way to cut down on the repetitive parts would make this way easier to read and reason about.
Potential idea: Wrap everything within the "if fx, ok := ..." blocks in a function that includes a lambda, where the funcx.Replace calls are made. That way the parts that actually vary (the Replace calls and any inputs for those) are more obvious, and the boilerplate of checking funcx.Satisfy is pared down.
There was a problem hiding this comment.
Done. This adds lines technically, but being able to more clearly avoid mismatches for name+signature pairs is probably worth it.
I threw it into a loop too.
| // They are not working examples. | ||
| type MyAccum struct{} | ||
|
|
||
| type GoodCombineFn struct{} |
There was a problem hiding this comment.
Nitpick, but all these structs blend together, making reading through this a pain for anyone that edits this file in the future. A way to break up the flow would be nice.
Thing is, the only way I can think of though is to add comments above each struct declaration, but the comments wouldn't really add any actual info, and it feels odd to suggest adding useless comments just to improve the formatting. Are there better alternatives?
There was a problem hiding this comment.
I added a comment here and there to break up the sections, but it doesn't add much or remove much. go fmt removes extra blank lines, and having empty // **** lines isn't common in Go.
|
Thanks for the comments! PTAL @youngoli |
|
R: @aaltay Please review and merge. Thank you! |
* [BEAM-6825] Improve Combine error messages. * !fixup Address style nits, and error readability.
* [BEAM-6825] Improve Combine error messages. * !fixup Address style nits, and error readability.
3 participants
The errors now more clearly outline what went wrong, with what, and what is expected. Improvements could be made, but this is a good step forward.
Now messages are more like:
"graph.AsCombineFn: <fully qualfied func/method path> must be a binary merge of accumulators to be a CombineFn. It is of type , but it must be of type func(context.Context?, A, A) (A, error?) where A is the accumulator type"
or graph.AsCombineFn invalid <method in question, like AddInput>: <fully qualified func/method path> has type , but expected "" to be the accumulator type ""; expected a signature like <general signature for the method in question, with the accumulator properly substituted in>"
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.