fix: use compaction-specific startup timeout (600s default)#115
Open
danielcherubini wants to merge 1 commit into
Open
fix: use compaction-specific startup timeout (600s default)#115danielcherubini wants to merge 1 commit into
danielcherubini wants to merge 1 commit into
Conversation
Contributor
WalkthroughThis PR adds a new ChangesCompaction Startup Timeout Configuration
Possibly Related PRs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/tama-core/src/config/types.rs (2)
849-858: ⚡ Quick winStrengthen default coverage for the new compaction startup timeout.
Line 857 verifies
timeout_msbut the defaults test does not assertstartup_timeout_secs, so regressions in the new default path can slip through.Suggested test additions
#[test] fn test_compaction_config_defaults() { let config = CompactionConfig::default(); assert!(!config.enabled); assert_eq!(config.server_path, None); assert_eq!(config.device, "cpu"); assert_eq!(config.port, None); assert_eq!(config.timeout_ms, 30_000); + assert_eq!(config.startup_timeout_secs, 600); } + +#[test] +fn test_compaction_config_startup_timeout_default_when_omitted() { + let config: CompactionConfig = toml::from_str( + r#" +enabled = true +"#, + ) + .unwrap(); + assert_eq!(config.startup_timeout_secs, 600); +}🤖 Prompt for 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. In `@crates/tama-core/src/config/types.rs` around lines 849 - 858, The test_compaction_config_defaults in CompactionConfig only asserts timeout_ms and omits asserting the new startup_timeout_secs default; update that test to also assert the expected default value for CompactionConfig::startup_timeout_secs (e.g. assert_eq!(config.startup_timeout_secs, <expected_value>)) so regressions to the startup timeout default are caught.
592-594: ⚡ Quick winRename the private helper to match the underscore-prefix convention.
default_compaction_startup_timeout_secsis private and currently violates the private-function naming rule.Suggested rename
-#[serde(default = "default_compaction_startup_timeout_secs")] +#[serde(default = "_default_compaction_startup_timeout_secs")] pub startup_timeout_secs: u64, ... -startup_timeout_secs: default_compaction_startup_timeout_secs(), +startup_timeout_secs: _default_compaction_startup_timeout_secs(), -fn default_compaction_startup_timeout_secs() -> u64 { +fn _default_compaction_startup_timeout_secs() -> u64 { 600 // 10 min — first-run dep install (torch, transformers) is slow }As per coding guidelines, "Prefix Rust private functions with
_(e.g.,_hf_api())."🤖 Prompt for 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. In `@crates/tama-core/src/config/types.rs` around lines 592 - 594, The private helper function default_compaction_startup_timeout_secs should be renamed to follow the private underscore-prefix rule; rename the function to _default_compaction_startup_timeout_secs and update all references (e.g., any #[serde(default = "default_compaction_startup_timeout_secs")] or direct calls) to use the new name so compilation and attribute links remain correct.Source: Coding guidelines
crates/tama-web/src/pages/config_editor.rs (1)
147-149: ⚡ Quick winRename this private helper to match the underscore-prefix naming convention.
As per coding guidelines, "Prefix Rust private functions with
_(e.g.,_hf_api())."🤖 Prompt for 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. In `@crates/tama-web/src/pages/config_editor.rs` around lines 147 - 149, Rename the private helper function default_compaction_startup_timeout_secs() to follow the underscore-prefix convention (e.g., _default_compaction_startup_timeout_secs) and update all call sites and references to that function (tests, modules, or other functions in the crate) to use the new name; ensure the function signature remains the same and run cargo fmt/build to verify no remaining references break.Source: Coding guidelines
crates/tama-web/src/types/config.rs (1)
312-314: ⚡ Quick winRename the private default helper to follow the underscore-prefix rule.
This helper is private and should use the configured
_prefix convention.As per coding guidelines, "Prefix Rust private functions with
_(e.g.,_hf_api())."🤖 Prompt for 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. In `@crates/tama-web/src/types/config.rs` around lines 312 - 314, Rename the private helper function default_compaction_startup_timeout_secs to follow the underscore-prefix convention (e.g., _default_compaction_startup_timeout_secs) and update all references to it (including any serde attributes or calls that refer to "default_compaction_startup_timeout_secs") so they point to the new function name; ensure the function signature and return value remain unchanged and run cargo check/tests to validate no remaining references.Source: Coding guidelines
🤖 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 `@crates/tama-core/src/proxy/lifecycle/mod.rs`:
- Around line 1299-1301: The assignment to `timeout` using
`std::time::Duration::from_secs(compaction.startup_timeout_secs)` is
misformatted; run the Rust formatter and commit the result: run `cargo fmt
--all` (or `rustfmt`) and reformat `crates/tama-core/src/proxy/lifecycle/mod.rs`
so the `let timeout =
std::time::Duration::from_secs(compaction.startup_timeout_secs);` hunk conforms
to rustfmt style, then stage and push the formatted changes.
---
Nitpick comments:
In `@crates/tama-core/src/config/types.rs`:
- Around line 849-858: The test_compaction_config_defaults in CompactionConfig
only asserts timeout_ms and omits asserting the new startup_timeout_secs
default; update that test to also assert the expected default value for
CompactionConfig::startup_timeout_secs (e.g.
assert_eq!(config.startup_timeout_secs, <expected_value>)) so regressions to the
startup timeout default are caught.
- Around line 592-594: The private helper function
default_compaction_startup_timeout_secs should be renamed to follow the private
underscore-prefix rule; rename the function to
_default_compaction_startup_timeout_secs and update all references (e.g., any
#[serde(default = "default_compaction_startup_timeout_secs")] or direct calls)
to use the new name so compilation and attribute links remain correct.
In `@crates/tama-web/src/pages/config_editor.rs`:
- Around line 147-149: Rename the private helper function
default_compaction_startup_timeout_secs() to follow the underscore-prefix
convention (e.g., _default_compaction_startup_timeout_secs) and update all call
sites and references to that function (tests, modules, or other functions in the
crate) to use the new name; ensure the function signature remains the same and
run cargo fmt/build to verify no remaining references break.
In `@crates/tama-web/src/types/config.rs`:
- Around line 312-314: Rename the private helper function
default_compaction_startup_timeout_secs to follow the underscore-prefix
convention (e.g., _default_compaction_startup_timeout_secs) and update all
references to it (including any serde attributes or calls that refer to
"default_compaction_startup_timeout_secs") so they point to the new function
name; ensure the function signature and return value remain unchanged and run
cargo check/tests to validate no remaining references.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 08df995e-2e20-45a3-8e83-9410b16b6926
📒 Files selected for processing (4)
crates/tama-core/src/config/types.rscrates/tama-core/src/proxy/lifecycle/mod.rscrates/tama-web/src/pages/config_editor.rscrates/tama-web/src/types/config.rs
Comment on lines
+1299
to
+1301
| // Use compaction-specific timeout (first-run dep install is slow) | ||
| let timeout = | ||
| std::time::Duration::from_secs(self.config.read().await.proxy.startup_timeout_secs); | ||
| std::time::Duration::from_secs(compaction.startup_timeout_secs); |
Contributor
There was a problem hiding this comment.
Run rustfmt on this hunk; CI is failing.
cargo fmt --all --check is currently failing for this file around this assignment, so this needs a formatting pass before merge.
As per coding guidelines, "Run cargo fmt --all for code formatting in Rust."
🤖 Prompt for 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.
In `@crates/tama-core/src/proxy/lifecycle/mod.rs` around lines 1299 - 1301, The
assignment to `timeout` using
`std::time::Duration::from_secs(compaction.startup_timeout_secs)` is
misformatted; run the Rust formatter and commit the result: run `cargo fmt
--all` (or `rustfmt`) and reformat `crates/tama-core/src/proxy/lifecycle/mod.rs`
so the `let timeout =
std::time::Duration::from_secs(compaction.startup_timeout_secs);` hunk conforms
to rustfmt style, then stage and push the formatted changes.
Sources: Coding guidelines, Pipeline failures
First-run uvx dependency install (torch, transformers) can take 5-10 min, which exceeds the proxy's 120s startup timeout. Added startup_timeout_secs to CompactionConfig with a 600s default, and updated lifecycle to use it. Also added the field to the web UI config editor.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix compaction server startup timeout — first-run
uvxdependency install (torch, transformers) can take 5-10 min, which exceeds the proxy's 120s startup timeout.Changes
startup_timeout_secsfield toCompactionConfig(default: 600s)Test plan
cargo clippy --workspace -- -D warnings— cleanSummary by CodeRabbit