Conversation
📝 WalkthroughWalkthroughThis set of changes restructures the speech-to-text (STT) and Whisper-related workspace crates by splitting the monolithic Changes
Sequence Diagram(s)Old Flow: Using Feature-Gated Modules in
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
crates/llama/Cargo.toml (1)
19-19: Consider branch maintenance strategy.Using a specific Git branch provides version control but requires maintenance to keep dependencies up-to-date.
Consider establishing a process to regularly update this branch reference or migrate to tagged releases when available.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/llama/Cargo.toml(1 hunks)crates/whisper/src/local/model.rs(1 hunks)plugins/local-llm/Cargo.toml(1 hunks)scripts/pre_build.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{js,ts,tsx,rs}`: 1. No error handling. 2. No unused imports, variables, or functions. 3. For comments, keep it minimal. It should be about "Why", not "What".
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
crates/whisper/src/local/model.rs
🪛 Ruff (0.11.9)
scripts/pre_build.py
83-83: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
162-162: Local variable vulkan_path is assigned to but never used
Remove assignment to unused variable vulkan_path
(F841)
163-163: Local variable vulkan_runtime_path is assigned to but never used
Remove assignment to unused variable vulkan_runtime_path
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ci
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci (macos, macos-latest)
🔇 Additional comments (3)
crates/whisper/src/local/model.rs (1)
49-51: ```shell
#!/bin/bashDisplay the beginning of the Whisper model file to review GPU parameter handling
sed -n '1,200p' crates/whisper/src/local/model.rs
</details> <details> <summary>plugins/local-llm/Cargo.toml (1)</summary> `10-15`: **LGTM! Clean feature flag implementation.** The feature flags properly delegate to the underlying `hypr-llama` dependency and maintain consistency with the centralized feature management approach. </details> <details> <summary>crates/llama/Cargo.toml (1)</summary> `7-12`: **LGTM! Unified feature flag approach.** The centralized feature flags replace platform-specific conditional dependencies, providing cleaner and more maintainable configuration. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
Cargo.toml(2 hunks)crates/stt/Cargo.toml(1 hunks)crates/stt/src/realtime/mod.rs(2 hunks)crates/stt/src/realtime/whisper.rs(1 hunks)crates/whisper-cloud/Cargo.toml(1 hunks)crates/whisper-cloud/src/client.rs(3 hunks)crates/whisper-local/Cargo.toml(1 hunks)crates/whisper-local/src/model.rs(5 hunks)crates/whisper-local/src/stream.rs(1 hunks)crates/whisper/Cargo.toml(0 hunks)crates/whisper/src/lib.rs(0 hunks)plugins/local-stt/Cargo.toml(1 hunks)plugins/local-stt/src/ext.rs(1 hunks)plugins/local-stt/src/server.rs(2 hunks)
💤 Files with no reviewable changes (2)
- crates/whisper/src/lib.rs
- crates/whisper/Cargo.toml
✅ Files skipped from review due to trivial changes (8)
- crates/whisper-local/src/stream.rs
- crates/stt/src/realtime/whisper.rs
- Cargo.toml
- crates/stt/src/realtime/mod.rs
- plugins/local-stt/src/server.rs
- crates/whisper-cloud/Cargo.toml
- crates/whisper-local/Cargo.toml
- crates/whisper-cloud/src/client.rs
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{js,ts,tsx,rs}`: 1. No error handling. 2. No unused imports, variables, or functions. 3. For comments, keep it minimal. It should be about "Why", not "What".
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
plugins/local-stt/src/ext.rscrates/whisper-local/src/model.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ci
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (4)
crates/stt/Cargo.toml (1)
14-14: LGTM: Dependency management improvements.The migration from path-based to workspace-based dependencies and the replacement of
hypr-whisperwith"cloud"feature to the dedicatedhypr-whisper-cloudcrate aligns well with the workspace restructuring objectives.Also applies to: 18-18, 19-19
plugins/local-stt/src/ext.rs (1)
182-182: Verify all references to the old whisper crate path have been updated.The namespace change from
hypr_whisper::localtohypr_whisper_localis consistent with the crate restructuring.#!/bin/bash # Description: Verify all references to hypr_whisper::local have been updated in the codebase # Expected: No remaining references to hypr_whisper::local echo "Checking for remaining references to hypr_whisper::local:" rg "hypr_whisper::local" --type rust echo -e "\nChecking for remaining references to hypr_whisper with local feature:" rg 'hypr-whisper.*local' --type tomlplugins/local-stt/Cargo.toml (1)
33-34: LGTM: Dependency split aligns with crate restructuring.The addition of
hypr-whisper-localas a separate dependency while keepinghypr-whisper(without the "local" feature) follows the expected pattern for the whisper crate split. This allows access to shared types from the base crate while using local-specific functionality from the dedicated crate.crates/whisper-local/src/model.rs (1)
11-11: LGTM: Proper dependency on shared Language type.The migration to use
Languagefrom thehypr_whispercrate instead of a local definition promotes code reuse and consistency across the whisper ecosystem.Also applies to: 20-20, 31-31, 65-65, 89-89
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/llama/src/lib.rs (1)
80-80: Simplify the redundant conditional expression.The conditional expression is redundant since
cfg!(debug_assertions)already returns a boolean.- let show_logs = if cfg!(debug_assertions) { true } else { false }; + let show_logs = cfg!(debug_assertions);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
apps/desktop/src-tauri/Cargo.toml(1 hunks)apps/desktop/src-tauri/build.rs(1 hunks)crates/llama/src/lib.rs(1 hunks)crates/whisper-local/Cargo.toml(1 hunks)crates/whisper-local/src/model.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/whisper-local/Cargo.toml
- crates/whisper-local/src/model.rs
- apps/desktop/src-tauri/Cargo.toml
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{js,ts,tsx,rs}`: 1. No error handling. 2. No unused imports, variables, or functions. 3. For comments, keep it minimal. It should be about "Why", not "What".
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
apps/desktop/src-tauri/build.rscrates/llama/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci
🔇 Additional comments (2)
crates/llama/src/lib.rs (1)
80-81: LGTM! Good conditional logging implementation.The logging configuration correctly enables logs only in debug builds, which is appropriate for performance and reduces noise in production environments.
apps/desktop/src-tauri/build.rs (1)
2-12: LGTM! Clean platform-specific feature detection.The implementation correctly detects the target OS and emits appropriate cargo configuration flags for platform-specific accelerator features. The logic aligns well with the PR objectives of tweaking accelerators by enabling
macos-defaultfeatures (Metal, CoreML) for macOS andwindows-defaultfeatures (Vulkan, OpenBLAS) for Windows.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
scripts/pre_build.py (4)
85-85: Fix exception chaining for better error context.The exception is raised without chaining the original exception, which loses the error context.
93-95: Add fallback for 7-Zip dependency.The script assumes 7-Zip is installed at a fixed path, which may not be true on all Windows systems.
164-165: Remove unused variables.These variables are assigned but never used, as flagged by static analysis.
186-186: Missing setup_openblas() call.The main function calls
setup_vulkan()but notsetup_openblas(), which means OpenBLAS configuration won't be applied to the Tauri config.
🧹 Nitpick comments (1)
scripts/pre_build.py (1)
88-139: Consider refactoring the setup_windows function.The function is quite long (51 lines) and handles multiple distinct responsibilities (OpenBLAS setup, Vulkan setup, vcpkg setup). Consider splitting it into smaller, focused functions.
Split into separate functions:
def setup_windows(): if has_feature("openblas"): setup_windows_openblas() if has_feature("vulkan"): setup_windows_vulkan() setup_windows_vcpkg() def setup_windows_openblas(): # OpenBLAS setup logic (lines 89-108) def setup_windows_vulkan(): # Vulkan setup logic (lines 110-133) def setup_windows_vcpkg(): # vcpkg setup logic (lines 135-138)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockcrates/pyannote-local/src/data/female_welcome_1.mp3is excluded by!**/*.mp3crates/pyannote-local/src/data/male_welcome_1.mp3is excluded by!**/*.mp3crates/pyannote-local/src/data/male_welcome_2.mp3is excluded by!**/*.mp3
📒 Files selected for processing (10)
Cargo.toml(2 hunks)apps/desktop/src-tauri/Cargo.toml(1 hunks)crates/onnx/Cargo.toml(1 hunks)crates/pyannote-cloud/Cargo.toml(1 hunks)crates/pyannote-local/Cargo.toml(1 hunks)crates/pyannote-local/src/embedding.rs(3 hunks)crates/pyannote/src/lib.rs(0 hunks)plugins/local-stt/Cargo.toml(2 hunks)plugins/local-stt/src/ext.rs(1 hunks)scripts/pre_build.py(1 hunks)
💤 Files with no reviewable changes (1)
- crates/pyannote/src/lib.rs
✅ Files skipped from review due to trivial changes (3)
- crates/pyannote-cloud/Cargo.toml
- crates/onnx/Cargo.toml
- crates/pyannote-local/src/embedding.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/local-stt/src/ext.rs
- Cargo.toml
- plugins/local-stt/Cargo.toml
- apps/desktop/src-tauri/Cargo.toml
🧰 Additional context used
🪛 Ruff (0.11.9)
scripts/pre_build.py
85-85: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
164-164: Local variable vulkan_path is assigned to but never used
Remove assignment to unused variable vulkan_path
(F841)
165-165: Local variable vulkan_runtime_path is assigned to but never used
Remove assignment to unused variable vulkan_runtime_path
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (macos, aarch64-apple-darwin, macos-latest)
- GitHub Check: build (macos, x86_64-apple-darwin, macos-latest)
- GitHub Check: ci
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (3)
crates/pyannote-local/Cargo.toml (3)
2-2: Crate rename consistency
The crate name changed topyannote-local. Ensure all workspace manifests, dependent crates (e.g.,hypr-pyannote-cloud), examples, and documentation are updated to referencepyannote-localinstead of the old name.
8-9: Clarify feature defaults and gating
Default features are empty here, so consumers must explicitly opt intocoremlordirectml. Confirm this design is intentional. Consider adding inline comments or README updates to document each feature’s purpose and usage.
14-16: Assess mandatory dependencies
dasp,knf-rs, andsimsimdwere moved from optional to required. If any of these are only needed for specific backends or workflows, consider gating them behind features to reduce unnecessary compilation overhead for other consumers.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/stt/Cargo.toml (1)
14-14: Consider feature-gating thehypr-clovadependency.The
hypr-clovacrate is currently pulled in unconditionally, yet it’s likely only needed under therecordedfeature. Making it optional and tying it to that feature will reduce compile times and binary size.Apply this diff:
[features] -recorded = [] +recorded = ["hypr-clova"] [dependencies] -hypr-clova = { workspace = true } +hypr-clova = { workspace = true, optional = true }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
apps/app/server/Cargo.toml(1 hunks)apps/desktop/src-tauri/src/lib.rs(1 hunks)crates/chunker/Cargo.toml(0 hunks)crates/clova/Cargo.toml(0 hunks)crates/db-script/Cargo.toml(0 hunks)crates/db-user/Cargo.toml(0 hunks)crates/llama/Cargo.toml(1 hunks)crates/pyannote-cloud/Cargo.toml(1 hunks)crates/rtzr/Cargo.toml(0 hunks)crates/stt/Cargo.toml(1 hunks)crates/turso/Cargo.toml(0 hunks)crates/vad/Cargo.toml(0 hunks)crates/whisper-cloud/Cargo.toml(1 hunks)crates/whisper-local/src/model.rs(5 hunks)crates/ws/Cargo.toml(0 hunks)plugins/analytics/Cargo.toml(0 hunks)plugins/apple-calendar/Cargo.toml(0 hunks)plugins/auth/Cargo.toml(0 hunks)plugins/connector/Cargo.toml(0 hunks)plugins/db/Cargo.toml(0 hunks)plugins/flags/Cargo.toml(0 hunks)plugins/local-llm/Cargo.toml(1 hunks)plugins/local-stt/Cargo.toml(2 hunks)plugins/misc/Cargo.toml(0 hunks)plugins/notification/Cargo.toml(0 hunks)plugins/sse/Cargo.toml(0 hunks)plugins/task/Cargo.toml(0 hunks)plugins/template/Cargo.toml(0 hunks)plugins/tray/Cargo.toml(0 hunks)
💤 Files with no reviewable changes (20)
- crates/db-user/Cargo.toml
- crates/clova/Cargo.toml
- plugins/analytics/Cargo.toml
- crates/ws/Cargo.toml
- plugins/auth/Cargo.toml
- plugins/sse/Cargo.toml
- plugins/template/Cargo.toml
- crates/turso/Cargo.toml
- crates/db-script/Cargo.toml
- plugins/task/Cargo.toml
- crates/vad/Cargo.toml
- plugins/apple-calendar/Cargo.toml
- crates/chunker/Cargo.toml
- plugins/tray/Cargo.toml
- plugins/db/Cargo.toml
- plugins/flags/Cargo.toml
- plugins/notification/Cargo.toml
- plugins/connector/Cargo.toml
- plugins/misc/Cargo.toml
- crates/rtzr/Cargo.toml
✅ Files skipped from review due to trivial changes (3)
- apps/desktop/src-tauri/src/lib.rs
- crates/whisper-cloud/Cargo.toml
- apps/app/server/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (5)
- plugins/local-llm/Cargo.toml
- crates/pyannote-cloud/Cargo.toml
- crates/whisper-local/src/model.rs
- plugins/local-stt/Cargo.toml
- crates/llama/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci
1 participant
No description provided.