chore: bump dependencies #226
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces multiple updates across several files, primarily focusing on dependency version upgrades in Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #226 +/- ##
==========================================
- Coverage 68.92% 68.60% -0.33%
==========================================
Files 13 13
Lines 943 946 +3
==========================================
- Hits 650 649 -1
- Misses 293 297 +4 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
src/algos/phenomizer.rs (1)
50-54: Consider being more explicit with Disease trait importThe wildcard import
Disease as _is typically discouraged as it can make the code less maintainable. Consider being explicit about which trait methods are needed.src/index.rs (1)
101-101: Consider pinning to a specific tantivy version rangeGiven that this is a critical indexing component handling medical ontology data, consider using a specific version range in your dependency specification (e.g.,
tantivy = "~0.x.y") to prevent unexpected breaking changes in patch updates.src/server/run/hpo_sim/term_term.rs (2)
Line range hint
171-171: Remove debug print statement.The
dbg!(&result)statement appears to be a development artifact. Consider removing it before deploying to production to avoid unnecessary logging and potential performance impact.- dbg!(&result);
Line range hint
122-139: Consider enhancing error handling.The current implementation has several areas where error handling could be improved:
- Invalid HPO terms are silently filtered out
- The sorting operation uses
expect()which could panic- No validation ensures non-empty input vectors
Consider:
- Returning an error if no valid terms are found
- Using proper error handling for sorting
- Validating input vectors are non-empty
Example improvement:
// Validate and convert terms with proper error handling let lhs = query .lhs .iter() .map(|lhs| { ontology .hpo(HpoTermId::from(lhs.clone())) .ok_or_else(|| CustomError::InvalidInput(format!("Invalid HPO term: {}", lhs))) }) .collect::<Result<Vec<_>, _>>()?; if lhs.is_empty() || rhs.is_empty() { return Err(CustomError::InvalidInput("Empty term sets provided".into())); } // Use proper error handling for sorting result.sort_by(|lhs, rhs| { rhs.score .partial_cmp(&lhs.score) .ok_or_else(|| CustomError::Internal("Failed to compare scores".into())) });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.toml(2 hunks)src/algos/phenomizer.rs(2 hunks)src/common.rs(4 hunks)src/convert/mod.rs(1 hunks)src/index.rs(1 hunks)src/query/mod.rs(6 hunks)src/server/run/hpo_genes.rs(1 hunks)src/server/run/hpo_omims.rs(2 hunks)src/server/run/hpo_sim/term_gene.rs(2 hunks)src/server/run/hpo_sim/term_term.rs(1 hunks)src/server/run/hpo_terms.rs(6 hunks)src/server/run/mod.rs(1 hunks)
🔇 Additional comments (20)
Cargo.toml (2)
18-18: Consider compile time impact of "full" feature
The addition of the "full" feature to derive_more could increase compile times. Consider using only the specific features needed instead of the full feature set if possible.
23-23: Verify compatibility with major version upgrades
Several dependencies have undergone major version updates which could introduce breaking changes:
- hpo: 0.8 -> 0.11 (3 major versions)
- utoipa-swagger-ui: 7.x -> 8.x
- utoipa: 4.x -> 5.x
Please ensure:
- All breaking changes have been addressed
- The application has been thoroughly tested with these new versions
- The changelog/release notes for these dependencies have been reviewed
Also applies to: 44-45
src/algos/phenomizer.rs (1)
91-91: Verify the change in expected test value
The test assertion value has changed from 1.757_194 to 1.756_347. Since this PR is about dependency updates, please verify if this change is:
- An expected result of the dependency updates
- A precision improvement
- A potential regression
Consider adding a comment explaining the reason for this specific value to help future maintenance.
src/server/run/hpo_sim/term_gene.rs (3)
14-17: LGTM! Import changes are well-structured.
The imports are properly organized and include the necessary modules for the new JSON response type.
57-57: LGTM! API documentation accurately reflects the response type.
The OpenAPI/Swagger documentation has been properly updated to match the new response type structure.
65-65: LGTM! Return type change improves API consistency.
The modification to return Json<query_result::Result> aligns with RESTful practices and maintains type safety while standardizing the API response format.
src/index.rs (1)
101-101: Type change looks correct but warrants testing verification
The update from tantivy::Document to tantivy::TantivyDocument appears to be part of a breaking change in the tantivy library. The change is implemented correctly, and the rest of the code continues to work with the new type, suggesting API compatibility is maintained.
Please ensure that:
- All tests pass with the new dependency version
- The search functionality works as expected, particularly the document indexing and retrieval
src/server/run/hpo_sim/term_term.rs (1)
118-118: LGTM! Good improvement to response type clarity.
The change from impl Responder to actix_web::Result<Json<Result>, CustomError> is a positive improvement that:
- Makes the API contract more explicit
- Ensures consistent JSON responses
- Maintains proper error handling
- Aligns with RESTful API best practices
src/common.rs (4)
69-69: LGTM: Display attribute syntax updated correctly for IcBasedOn enum
The display attribute syntax has been updated from #[display(fmt = "...")] to #[display("...")] consistently for both variants, maintaining the same string values.
Also applies to: 72-72
108-108: LGTM: Display attribute syntax updated correctly for SimilarityMethod enum
The display attribute syntax has been updated consistently across all variants of the SimilarityMethod enum, preserving the original string representations.
Also applies to: 111-111, 114-114, 117-117, 120-120, 123-123, 126-126, 130-130
185-185: LGTM: Display attribute syntax updated correctly for ScoreCombiner enum
The display attribute syntax has been updated consistently for all variants of the ScoreCombiner enum, maintaining the same string values.
Also applies to: 188-188, 191-191
200-200: LGTM: Fixed incorrect mapping in From implementation
The mapping for ScoreCombiner::Bma has been correctly updated to map to StandardCombiner::Bma instead of the previous incorrect StandardCombiner::Bwa.
src/query/mod.rs (2)
55-63: LGTM! Clean reordering of derive attributes
The reordering of derive attributes improves code organization by grouping similar traits together. This is a good practice in Rust and aligns with the dependency update objectives.
269-271: LGTM! Improved logging level check
The simplified pattern matching for log levels is more idiomatic Rust code and improves readability while maintaining the same functionality.
src/server/run/hpo_genes.rs (1)
145-145: LGTM: Return type standardization aligns with RESTful practices
The change from impl Responder to Json<Result> is a good improvement that makes the API response type more explicit and consistent with RESTful practices.
src/server/run/hpo_omims.rs (1)
180-180: LGTM! Return type change improves API contract clarity.
The change from impl Responder to Json<Result> makes the API contract more explicit and aligns with RESTful best practices. This change is consistent with similar modifications across other endpoints, improving the overall API consistency.
src/server/run/hpo_terms.rs (4)
10-10: LGTM: Import needed for tantivy 0.22 compatibility
The explicit import of the Value trait is required for the tantivy 0.22 upgrade to support the as_value() method calls.
108-108: LGTM: Correct tantivy 0.22 API usage
The changes properly adapt to tantivy 0.22's API:
- Updated document type from Document to TantivyDocument
- Updated value access patterns using as_value()
Also applies to: 150-150, 155-155, 164-164
218-218: LGTM: Consistent JSON response type
The changes properly standardize the API response to use JSON format, which:
- Improves API consistency
- Follows RESTful best practices
Also applies to: 226-226
329-340: LGTM: Proper tantivy 0.22 document retrieval
The changes correctly implement tantivy 0.22's document retrieval:
- Uses explicit TantivyDocument type
- Maintains comprehensive error handling
- Properly accesses document values
| tantivy = "0.21.1" | ||
| tempdir = "0.3.7" | ||
| tantivy = "0.22" | ||
| tempdir = "0.3" |
There was a problem hiding this comment.
Verify tempdir version change
The tempdir version change from 0.3.7 to 0.3 appears to be a downgrade. Please verify if this is intentional or if it should be 0.3.x where x ≥ 7.
| if let Some(log::Level::Trace | log::Level::Debug) = args_common.verbose.log_level() { | ||
| std::env::set_var("RUST_LOG", "debug"); | ||
| env_logger::init_from_env(env_logger::Env::new().default_filter_or("info")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider revising the logging configuration approach
The current implementation has potential issues:
- There's a conflict between setting
RUST_LOG=debugand using"info"as the default filter - Setting environment variables at runtime could lead to race conditions in concurrent contexts
Consider this more idiomatic approach:
- if let Some(log::Level::Trace | log::Level::Debug) = args_common.verbose.log_level() {
- std::env::set_var("RUST_LOG", "debug");
- env_logger::init_from_env(env_logger::Env::new().default_filter_or("info"));
+ if let Some(level) = args_common.verbose.log_level() {
+ env_logger::Builder::new()
+ .filter_level(level.to_level_filter())
+ .init();This approach:
- Directly uses the log level from args without environment variables
- Maintains consistency between the requested and applied log levels
- Is more thread-safe
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some(log::Level::Trace | log::Level::Debug) = args_common.verbose.log_level() { | |
| std::env::set_var("RUST_LOG", "debug"); | |
| env_logger::init_from_env(env_logger::Env::new().default_filter_or("info")); | |
| if let Some(level) = args_common.verbose.log_level() { | |
| env_logger::Builder::new() | |
| .filter_level(level.to_level_filter()) | |
| .init(); |
| if let Some(log::Level::Trace | log::Level::Debug) = args_common.verbose.log_level() { | ||
| std::env::set_var("RUST_LOG", "debug"); | ||
| env_logger::init_from_env(env_logger::Env::new().default_filter_or("info")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adjusting log level consistency
The pattern matching improvement is good, but there are potential issues with log level consistency:
- When verbosity is set to
Trace, RUST_LOG is set to "debug" which might be too restrictive - The default filter of "info" in
default_filter_or("info")might override the "debug" setting
Consider this adjustment to maintain log level consistency:
- if let Some(log::Level::Trace | log::Level::Debug) = args_common.verbose.log_level() {
- std::env::set_var("RUST_LOG", "debug");
- env_logger::init_from_env(env_logger::Env::new().default_filter_or("info"));
+ if let Some(level) = args_common.verbose.log_level() {
+ match level {
+ log::Level::Trace => std::env::set_var("RUST_LOG", "trace"),
+ log::Level::Debug => std::env::set_var("RUST_LOG", "debug"),
+ _ => {}
+ }
+ env_logger::init_from_env(env_logger::Env::new().default_filter_or("info"));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some(log::Level::Trace | log::Level::Debug) = args_common.verbose.log_level() { | |
| std::env::set_var("RUST_LOG", "debug"); | |
| env_logger::init_from_env(env_logger::Env::new().default_filter_or("info")); | |
| if let Some(level) = args_common.verbose.log_level() { | |
| match level { | |
| log::Level::Trace => std::env::set_var("RUST_LOG", "trace"), | |
| log::Level::Debug => std::env::set_var("RUST_LOG", "debug"), | |
| _ => {} | |
| } | |
| env_logger::init_from_env(env_logger::Env::new().default_filter_or("info")); |
Summary by CodeRabbit
New Features
Bug Fixes
ScoreCombiner::Bmato ensure accurate functionality.Refactor
Documentation