feat: support /v1/audio/speech api request.#1272
feat: support /v1/audio/speech api request.#1272wxh571001500 wants to merge 3 commits intojd-opensource:mainfrom
Conversation
wxh571001500
requested review from
DongheJin,
JimHsiung,
RobbieLeung,
XuZhang99,
liutongxuan,
walsonyang and
yq33victor
as code owners
wxh571001500
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a new speech service, adding the /v1/audio/speech endpoint and supporting infrastructure including SpeechServiceImpl, protobuf definitions, and request routing logic. Review feedback highlights several violations of the project's style guide, specifically the improper use of auto for primitive types, the lack of parameter annotations for constant arguments, and the use of push_back instead of emplace_back. Furthermore, a functional issue was identified in the speech processing logic where the service fails to return audio data upon completion, instead returning a placeholder error message.
| done_guard.release(), | ||
| const_cast<proto::SpeechRequest*>(request), | ||
| response, | ||
| true); |
There was a problem hiding this comment.
Annotate constant arguments with a comment indicating the parameter name when calling functions or constructors to improve readability.
/*use_arena=*/true);References
- Annotate constant arguments with a comment indicating the parameter name when calling functions or constructors. (link)
| json2pb::Json2PbOptions options; | ||
| butil::IOBuf& buf = ctrl->request_attachment(); | ||
| butil::IOBufAsZeroCopyInputStream iobuf_stream(buf); | ||
| auto st = json2pb::JsonToProtoMessage(&iobuf_stream, req_pb, options, &error); |
There was a problem hiding this comment.
Do not use auto for simple/primitive types like bool. Explicitly state the type to maintain clarity.
bool st = json2pb::JsonToProtoMessage(&iobuf_stream, req_pb, options, &error);References
- Do not use
autofor simple/primitive types (int32_t, float, bool, std::string, etc.). (link)
| segments.reserve(seg_list.segments_size()); | ||
| for (const auto& proto_seg : seg_list.segments()) { | ||
| segments.emplace_back(proto_seg.offset(), proto_seg.size()); | ||
| segments.push_back({proto_seg.offset(), proto_seg.size()}); |
There was a problem hiding this comment.
Prefer emplace_back over push_back to construct elements in-place and avoid unnecessary copies or temporary objects.
segments.emplace_back(proto_seg.offset(), proto_seg.size());References
- Prefer
emplace_backoverpush_backto construct elements in-place and avoid unnecessary copies. (link)
| RequestParams request_params( | ||
| request, call->get_x_request_id(), call->get_x_request_time()); | ||
|
|
||
| auto prompt = request.input(); |
There was a problem hiding this comment.
Do not use auto for std::string. Use the explicit type to adhere to the project's coding style.
| auto prompt = request.input(); | |
| std::string prompt = request.input(); |
References
- Do not use
autofor simple/primitive types (int32_t, float, bool, std::string, etc.). (link)
| if (req_output.finished || req_output.cancelled) { | ||
| return call->finish_with_error(StatusCode::UNAVAILABLE, | ||
| kRuntimeUnavailableMessage); | ||
| } |
There was a problem hiding this comment.
The current implementation does not extract audio data from req_output and always returns an error when the request is finished. This prevents the API from actually delivering speech output. Please ensure the audio data is correctly extracted and populated in the response.
wxh571001500
force-pushed
the
speech-api
branch
from
dbb95db to
5b23dd6
Compare
|
|
||
| const char* kSampleNotSupportedError = "/v1/sample is only supported for LLM"; | ||
| std::string build_sample_backend_error_message() { | ||
| return "Current backend '" + FLAGS_backend + |
There was a problem hiding this comment.
这个FLAGS_backend已经全局删除了
wxh571001500
force-pushed
the
speech-api
branch
2 times, most recently
from
667caf5 to
b80901f
Compare
wxh571001500
force-pushed
the
speech-api
branch
from
b80901f to
4c59440
Compare
| ::google::protobuf::Closure* done) { | ||
| xllm::ClosureGuard done_guard( | ||
| done, | ||
| std::bind(request_in_metric, nullptr), |
There was a problem hiding this comment.
use lambda func instead of std::bind
| xllm::ClosureGuard done_guard( | ||
| done, | ||
| std::bind(request_in_metric, nullptr), | ||
| std::bind(request_out_metric, (void*)controller)); |
3 participants
No description provided.