fix(whisper): honour positional listen address argument - #11652
fix(whisper): honour positional listen address argument#11652localai-org-maint-bot merged 2 commits into
Conversation
The whisper backend parsed its gRPC listen address exclusively through Go's flag package, while run.sh forwards launcher arguments verbatim. A bare positional address was silently dropped by flag.Parse(), so the server always bound the default localhost:50051 instead of the port its caller allocated — LocalAI then failed to reach it with a misleading 'error reading from server: EOF'. Fall back to the first positional argument when no explicit -addr value was given, keeping the default for no-argument launches. Fixes mudler#11623 Assisted-by: ox-alpha:ox-alpha [go test] Signed-off-by: Som Samantray <som.samantray@gmail.com>
…st style Review follow-up: - Detect an explicitly set -addr with flag.FlagSet.Visit instead of comparing against the default sentinel, so '-addr localhost:50051' plus a positional argument keeps the flag value. - Treat an explicitly empty -addr as unset rather than binding the empty address (OS-chosen port on all interfaces). - Rewrite addr_test.go as Ginkgo v2 specs per .agents/coding-style.md; stdlib t.Run/t.Errorf are forbidden by .golangci.yml forbidigo. Assisted-by: ox-alpha:ox-alpha [go test] Signed-off-by: Som Samantray <som.samantray@gmail.com>
localai-org-maint-bot
left a comment
There was a problem hiding this comment.
The positional-address fallback preserves explicit -addr precedence and the existing no-argument default, and the focused specs cover the edge cases. DCO passes; normal CI has not reported yet. The targeted package test is currently blocked by generated protobuf symbols missing on the base branch, unrelated to this two-file diff. Good to merge once the remaining checks are green. @mudler
|
Review pass. The code is clean and the specs are proper Ginkgo, but I could not find the failure mode in the tree, so this needs a maintainer call on whether it is fixing a real in-product bug. What I checked. Every backend spawn passes the address as a flag: The reproduction in the issue is a manual On the change itself: Consistency question: DCO is clean on both commits. |
|
I don't think this is the right fix, because the premise in #11623 doesn't hold: LocalAI does not pass the listen address positionally.
process.WithArgs(append(args, []string{"--addr", serverAddress}...)...)That is a normal flag, which The reproduction in the issue is: ./run.sh 127.0.0.1:59999That is a hand invocation with a bare positional argument. LocalAI itself invokes Two consequences:
The Worth getting @kamilsa to re-check with |
Description
This PR fixes #11623
The whisper backend parsed its gRPC listen address exclusively through Go's
flagpackage (-addr), whilebackend/go/whisper/run.shforwards launcher arguments verbatim (exec "$CURDIR"/whisper "$@"). A bare positional address was silently dropped byflag.Parse(), so the server always bound the defaultlocalhost:50051instead of the port LocalAI actually allocated — LocalAI then connected to its allocated port, found nothing there, and failed with a misleadingrpc error: code = Unavailable ... EOF.What changed
backend/go/whisper/main.go: newresolveAddrhelper that resolves the listen address in this order:-addrflag (detected viaflag.FlagSet.Visit, so-addr localhost:50051explicitly still wins over a positional),localhost:50051) for no-argument launches.An explicitly empty
-addr=""is treated as unset rather than binding an OS-chosen port on all interfaces.backend/go/whisper/addr_test.go: Ginkgo v2 specs covering all resolution branches (explicit flag wins, explicit-default wins over positional, positional fallback, no-arg default, first-positional-wins, empty-flag-as-unset).Scope note: 31 other Go backends under
backend/go/share the same flag-only parsing pattern and could hit the same failure. This PR intentionally keeps the fix scoped to the reported backend; happy to follow up with a shared helper for the rest if maintainers agree.Testing
go build ./backend/go/whisper/— cleango test ./backend/go/whisper/ -count=1— all specs pass (6 new address-resolution specs + existing suite)go vet ./backend/go/whisper/— clean (one pre-existing warning in untouchedgowhisper.go)gofmt -l backend/go/whisper/— cleanNotes for Reviewers
main()uses; a full runtime smoke of the built binary requires natively buildinglibgowhisper(cmake build of whisper.cpp) and was not run here.Assisted-by:commit trailers. All code was reviewed and tested by the human submitter.Signed commits