Add doc for Python API - #3627
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdds file-local docstring constants and attaches them to pybind11 registrations across many C++ modules; also expands Python facade class/module/method docstrings. No runtime behavior or public API signatures were changed. ChangesPython API Documentation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dosubot
Bot
added
the
size:XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sherpa-onnx/python/csrc/keyword-spotter.cc`:
- Around line 58-67: The docstring constant kCreateStreamDoc documents a
`keywords` parameter but is currently attached to the zero-argument overload of
create_stream, producing incorrect Python help; update the binding so the
docstring matches the correct overload or revise kCreateStreamDoc to describe
the zero-arg create_stream signature. Locate the create_stream overload bindings
in keyword-spotter.cc and either (a) move/apply kCreateStreamDoc to the overload
that accepts the keywords parameter, or (b) change kCreateStreamDoc text to
remove the `keywords` section so it accurately documents the no-argument
create_stream overload referenced by the binding.
In `@sherpa-onnx/python/csrc/offline-speaker-diarization.cc`:
- Around line 43-45: The docstring promises that a non-zero return from the
progress callback will abort processing but the progress-callback wrapper in
offline-speaker-diarization.cc always returns 0; fix this by propagating the
callback's return value: change the wrapper that invokes
callback(processed_chunks, num_chunks) to return the callback's int result (and
ensure the caller checks for non-zero and aborts), or if you prefer not to
support aborting, update the docstring to remove the "non-zero abort" claim;
reference the callback symbol ("callback(processed_chunks, num_chunks)") and the
progress-callback wrapper in offline-speaker-diarization.cc when making the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cadb656b-c483-48f8-8e24-f7f6a5dbaa1b
📒 Files selected for processing (28)
sherpa-onnx/python/csrc/audio-tagging.ccsherpa-onnx/python/csrc/circular-buffer.ccsherpa-onnx/python/csrc/display.ccsherpa-onnx/python/csrc/fast-clustering.ccsherpa-onnx/python/csrc/keyword-spotter.ccsherpa-onnx/python/csrc/offline-model-config.ccsherpa-onnx/python/csrc/offline-punctuation.ccsherpa-onnx/python/csrc/offline-recognizer.ccsherpa-onnx/python/csrc/offline-source-separation.ccsherpa-onnx/python/csrc/offline-speaker-diarization.ccsherpa-onnx/python/csrc/offline-speech-denoiser.ccsherpa-onnx/python/csrc/offline-stream.ccsherpa-onnx/python/csrc/offline-tts.ccsherpa-onnx/python/csrc/online-model-config.ccsherpa-onnx/python/csrc/online-punctuation.ccsherpa-onnx/python/csrc/online-recognizer.ccsherpa-onnx/python/csrc/online-speech-denoiser.ccsherpa-onnx/python/csrc/online-stream.ccsherpa-onnx/python/csrc/speaker-embedding-extractor.ccsherpa-onnx/python/csrc/speaker-embedding-manager.ccsherpa-onnx/python/csrc/spoken-language-identification.ccsherpa-onnx/python/csrc/version.ccsherpa-onnx/python/csrc/voice-activity-detector.ccsherpa-onnx/python/csrc/wave-writer.ccsherpa-onnx/python/sherpa_onnx/display.pysherpa-onnx/python/sherpa_onnx/keyword_spotter.pysherpa-onnx/python/sherpa_onnx/offline_recognizer.pysherpa-onnx/python/sherpa_onnx/online_recognizer.py
![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 significantly improves the documentation for the Python API by adding comprehensive docstrings and usage examples to both the C++ bindings and Python wrapper classes across numerous modules, including speech recognition, speaker diarization, and audio tagging. Feedback from the review identifies several issues in the newly added documentation, specifically highlighting incorrect code examples in online_recognizer.py that would cause attribute errors and mismatched docstrings for overloaded methods in the OnlineRecognizer and KeywordSpotter C++ bindings.
| result = recognizer.get_result(stream) | ||
| print(result.text) |
There was a problem hiding this comment.
The example code is incorrect because recognizer.get_result(stream) returns a str (as defined in line 1104), which does not have a .text attribute. To access the result object and its properties, recognizer.get_result_all(stream) should be used instead.
| result = recognizer.get_result(stream) | |
| print(result.text) | |
| result = recognizer.get_result_all(stream) | |
| print(result.text) |
| result = recognizer.get_result(stream) | ||
| print("Endpoint detected:", result.text) |
There was a problem hiding this comment.
Same as the previous issue: recognizer.get_result(stream) returns a string, so accessing .text on it will raise an AttributeError. Use get_result_all(stream) to obtain the result object.
| result = recognizer.get_result(stream) | |
| print("Endpoint detected:", result.text) | |
| result = recognizer.get_result_all(stream) | |
| print("Endpoint detected:", result.text) |
| static constexpr const char *kCreateStreamDoc = R"doc( | ||
| Create a new ``OnlineStream`` for decoding. | ||
|
|
||
| Args: | ||
| hotwords: | ||
| Optional hotwords for this stream. If provided, it is a string of | ||
| hotwords separated by ``/``. | ||
| Return: | ||
| An ``OnlineStream`` object. | ||
| )doc"; |
There was a problem hiding this comment.
The docstring kCreateStreamDoc describes a hotwords argument, but it is being applied to the no-argument overload of create_stream at line 222. This results in misleading documentation for the Python API. It is better to split this into two separate docstrings to accurately reflect the arguments for each overload, following the pattern used in offline-recognizer.cc.
static constexpr const char *kCreateStreamDoc = R"doc(
Create a new OnlineStream for decoding.
Return:
An OnlineStream object.
)doc";
static constexpr const char *kCreateStreamHotwordsDoc = R"doc(
Create a new OnlineStream for decoding.
Args:
hotwords:
Optional hotwords for this stream. If provided, it is a string of
hotwords separated by /.
Return:
An OnlineStream object.
)doc";| }, | ||
| py::arg("hotwords"), py::call_guard<py::gil_scoped_release>()) | ||
| .def("is_ready", &PyClass::IsReady, | ||
| py::arg("hotwords"), kCreateStreamDoc, |
There was a problem hiding this comment.
Use the new docstring that correctly describes the hotwords argument for this overload.
| py::arg("hotwords"), kCreateStreamDoc, | |
| py::arg("hotwords"), kCreateStreamHotwordsDoc, |
| static constexpr const char *kCreateStreamDoc = R"doc( | ||
| Create a new streaming recognition instance. | ||
|
|
||
| Args: | ||
| keywords: | ||
| A string of keywords to spot, separated by ``/``. | ||
|
|
||
| Returns: | ||
| An ``OnlineStream`` object. | ||
| )doc"; |
There was a problem hiding this comment.
The docstring kCreateStreamDoc describes a keywords argument but is applied to the no-argument overload at line 115. Conversely, the overload that actually takes keywords at line 121 is missing the docstring. These should be split to ensure accurate documentation for both overloads.
static constexpr const char *kCreateStreamDoc = R"doc(
Create a new streaming recognition instance.
Returns:
An OnlineStream object.
)doc";
static constexpr const char *kCreateStreamKeywordsDoc = R"doc(
Create a new streaming recognition instance.
Args:
keywords:
A string of keywords to spot, separated by /.
Returns:
An OnlineStream object.
)doc";| [](PyClass &self, const std::string &keywords) { | ||
| return self.CreateStream(keywords); | ||
| }, | ||
| py::arg("keywords"), py::call_guard<py::gil_scoped_release>()) |
There was a problem hiding this comment.
Add the docstring that correctly describes the keywords argument for this overload.
| py::arg("keywords"), py::call_guard<py::gil_scoped_release>()) | |
| py::arg("keywords"), kCreateStreamKeywordsDoc, py::call_guard<py::gil_scoped_release>()) |
1 participant
Summary by CodeRabbit