feat: parallelize suggestion requests #185
Conversation
SPE-1845 Parallelize suggestions
We can spend upwards of 5 min on a 10 error, <150 line spec. Let's run suggestions in parallel to speed this up |
|
|
||
| /** | ||
| * Non-parallelized suggestions | ||
| */ |
There was a problem hiding this comment.
I imagine we'll just remove this soon. Don't think there's a reason to not parallelize
There was a problem hiding this comment.
Agree, let's just leave it in the interim until we have confidence in parallelization
out.yaml
Outdated
| @@ -0,0 +1,150 @@ | |||
| openapi: 3.0.0 | |||
There was a problem hiding this comment.
did you mean to commit this?
cmd/suggest.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| if !slices.Contains([]string{"error", "warn", "info"}, level) { |
There was a problem hiding this comment.
may be better to use errors.Severity enum here instead of magic strings: https://github.com/speakeasy-api/openapi-generation/blob/main/pkg/errors/errors.go#L12
| @@ -76,7 +93,7 @@ func suggestFixesOpenAPI(cmd *cobra.Command, args []string) error { | |||
| suggestionConfig.MaxSuggestions = &maxSuggestion | |||
| } | |||
|
|
|||
| if err := validation.ValidateOpenAPI(cmd.Context(), schemaPath, &suggestionConfig, true); err != nil { | |||
| if err := suggestions.StartSuggest(cmd.Context(), schemaPath, &suggestionConfig, true); err != nil { | |||
| return err | ||
| } | ||
|
|
||
| if suggestionsConfig.OutputFile != "" && suggestionsConfig.AutoContinue { |
There was a problem hiding this comment.
Why does AutoContinue have to be enabled for this log statement to be output?
There was a problem hiding this comment.
Because if it isn't then you'll have the y/n every step of the way
There was a problem hiding this comment.
I'm a bit confused, because I'm not sure how removing this changes the y/n step logic. Isn't that done here?
speakeasy/internal/suggestions/suggestions.go
Lines 85 to 87 in a0e8fbf
There was a problem hiding this comment.
basically if you have on "autocontinue" it will just print out all the suggestions and then end, which makes it seem like it didn't write anything but actually it was writing to the output file all along. So I just added this message to make it clear that if you have -a and -o it still did the writing
| }() | ||
| // read from channel as they come in until its closed | ||
| for res := range ch { | ||
| suggestions[res.errorNum] = res.suggestion |
There was a problem hiding this comment.
perhaps my lack of go skill is showing, but what happens with the last error suggestion -- does it successfully get recorded here?
Just wondering if there's any concurrency issues because I'd imagine as soon as wg.Done gets called for that, both close(ch) in the other goroutine and this line in the for loop would execute, right? So I'm imagining there's a chance it can sometimes get dropped?
There was a problem hiding this comment.
Im definitely not an expert in goroutines by any stretch but my understanding is that this is a standard pattern (I got this from somewhere online)
There was a problem hiding this comment.
I think it should be fine because ch <- executes before wg.Done, so ideally the main thread should be able to pull of the channel before it gets closed. So yeah this is probably fine and we can revisit if we notice anything weird about last error
chase-crumbaugh
force-pushed
the
feat/spe-1845
branch
from
558b6fe
to
710f294
Compare
Requests all suggestions in parallel, then prints them out in order once all suggestions have been received. This works more or less with the current LLM server, but will benefit a lot from changes there to improve fault tolerance (e.g. when we get rate limited by openai)
This PR also contains a refactor to disentangle the validate and suggest CLI workflows
Server PR: https://github.com/speakeasy-api/speakeasy-registry/pull/1136