LNK-2950: Ability to run measure eval using a command line interface #460
Conversation
…ing a data from the locally running file system
WalkthroughThe pull request introduces a Changes
Assessment against linked issues
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Java/measureeval/src/main/java/com/lantanagroup/link/measureeval/FileSystemInvocation.java
Outdated
Show resolved
Hide resolved
Java/measureeval/src/main/java/com/lantanagroup/link/measureeval/FileSystemInvocation.java
Outdated
Show resolved
Hide resolved
Java/measureeval/src/main/java/com/lantanagroup/link/measureeval/FileSystemInvocation.java
Outdated
Show resolved
Hide resolved
Java/measureeval/src/main/java/com/lantanagroup/link/measureeval/FileSystemInvocation.java
Outdated
Show resolved
Hide resolved
Java/measureeval/src/main/java/com/lantanagroup/link/measureeval/FileSystemInvocation.java
Show resolved
Hide resolved
Java/measureeval/src/main/java/com/lantanagroup/link/measureeval/FileSystemInvocation.java
Outdated
Show resolved
Hide resolved
Java/measureeval/src/main/java/com/lantanagroup/link/measureeval/FileSystemInvocation.java
Outdated
Show resolved
Hide resolved
Java/measureeval/src/main/java/com/lantanagroup/link/measureeval/FileSystemInvocation.java
Outdated
Show resolved
Hide resolved
Java/measureeval/src/main/java/com/lantanagroup/link/measureeval/FileSystemInvocation.java
Show resolved
Hide resolved
Java/measureeval/src/main/java/com/lantanagroup/link/measureeval/FileSystemInvocation.java
Outdated
Show resolved
Hide resolved
Java/measureeval/src/main/java/com/lantanagroup/link/measureeval/FileSystemInvocation.java
Outdated
Show resolved
Hide resolved
Java/measureeval/src/main/java/com/lantanagroup/link/measureeval/FileSystemInvocation.java
Outdated
Show resolved
Hide resolved
Java/measureeval/src/main/java/com/lantanagroup/link/measureeval/FileSystemInvocation.java
Outdated
Show resolved
Hide resolved
…asure and test data Extending the documentation into a README.md on the root of the measureeval project
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
Java/measureeval/src/main/java/com/lantanagroup/link/measureeval/FileSystemInvocation.java (3)
28-90: LGTM with minor suggestions!The
getBundlemethod effectively handles loading measure bundles from both files and directories. The logic for parsing JSON and XML formats and recursively loading resources from directories is implemented correctly.Suggestions for minor improvements:
- Use the two-argument version of
readFileToStringto specify the charset explicitly, otherwise it defaults to the system charset which may not always be UTF-8.- Consider parsing directly from a
FileInputStreaminstead of first reading the entire file into aString. This can be more memory-efficient, especially for large files.
128-134: Looks good with a suggestion!The
findPatientmethod correctly uses streams to find the first Patient resource in a Bundle and throws an appropriate exception if no Patient resource is found.Suggestion:
- Consider throwing an exception if more than one Patient resource is found in the Bundle. You can replace
findFirst()withreduce(StreamUtils::toOnlyElement)to achieve this.
147-179: Looks good with a minor suggestion!The
mainmethod effectively orchestrates the measure evaluation process. It validates the command-line arguments, loads the measure and patient bundles, and evaluates the patient bundle(s) using the compiledMeasureEvaluator. The method handles both single file and directory patient bundle paths and logs detailed evaluation results for each patient.Suggestion:
- Consider wrapping the argument parsing logic (lines 153-156) in a try-catch block and print usage information to stderr if it fails. This will provide a more user-friendly experience if the arguments are not provided correctly.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- Java/measureeval/README.md (1 hunks)
- Java/measureeval/src/main/java/com/lantanagroup/link/measureeval/FileSystemInvocation.java (1 hunks)
- Java/measureeval/src/main/java/com/lantanagroup/link/measureeval/services/MeasureEvaluator.java (3 hunks)
- Java/measureeval/src/main/resources/logback.xml (1 hunks)
Additional context used
Learnings (1)
Java/measureeval/src/main/java/com/lantanagroup/link/measureeval/FileSystemInvocation.java (1)
Learnt from: seanmcilvenna PR: lantanagroup/link-cloud#460 File: Java/measureeval/src/main/java/com/lantanagroup/link/measureeval/FileSystemInvocation.java:13-22 Timestamp: 2024-09-17T21:49:24.887Z Learning: When reviewing class documentation, ensure that suggested enhancements are not already covered in the existing comments.
Additional comments not posted (10)
Java/measureeval/src/main/resources/logback.xml (3)
3-9: LGTM!The console appender is correctly configured with a
PatternLayoutthat includes relevant information for debugging and monitoring. The pattern layout is well-structured and readable.
11-13: LGTM!The loggers for
com.lantanagroup.linkandorg.opencds.cqfpackages are correctly configured with appropriate log levels. Referencing the "CONSOLE" appender ensures that the logs are output to the console as expected.Also applies to: 15-17
19-21: LGTM!The root logger is correctly configured to log messages at the "error" level and directs the output to the "CONSOLE" appender. This ensures that all error messages are captured and logged to the console.
Java/measureeval/README.md (1)
1-38: Excellent documentation for themeasureevalmodule!The
README.mdfile provides clear and comprehensive instructions for building and running themeasureevalmodule as a CLI. It includes:
- Steps to compile the module using Maven
- Instructions for executing the JAR file with specific parameters
- A table describing the required parameters
- An example command demonstrating how to structure the command line input
The well-formatted and easy-to-follow documentation enhances user understanding and accessibility of the
measureevalfunctionality, facilitating its use in practical scenarios.Java/measureeval/src/main/java/com/lantanagroup/link/measureeval/services/MeasureEvaluator.java (4)
32-34: LGTM!The new public constructor provides a convenient way to create
MeasureEvaluatorinstances with debug logging disabled by default. It delegates to the private constructor with the appropriate arguments.
Line range hint
36-49: LGTM!The modification to the private constructor allows controlling the debug logging behavior for the CQL engine based on the
isDebugflag. This provides flexibility in enabling or disabling debug logging as needed.
61-62: LGTM!The update to the
compilemethod maintains backward compatibility by delegating to the overloaded version withisDebugset tofalse. This ensures that the existing behavior is preserved when theisDebugparameter is not provided.
64-65: LGTM!The new overloaded
compilemethod provides a way to createMeasureEvaluatorinstances with the ability to control the debug logging behavior. It uses the private constructor with theisDebugflag to enable or disable debug logging as desired.Java/measureeval/src/main/java/com/lantanagroup/link/measureeval/FileSystemInvocation.java (2)
93-115: Looks good!The
getBundlesFromDirectoryAndSubDirectoriesmethod correctly implements the recursive loading of bundles from a directory and its subdirectories. Filtering files based on extensions and handling exceptions during bundle loading is done appropriately.
117-126: LGTM!The
getGroupPopulationsmethod correctly extracts and formats the group and population information from a MeasureReport into a human-readable string. The implementation looks good.
|
@smailliwcs In this round of changes I added a |
|
Pushed a commit to allow choosing (at build time via a Maven profile) whether to use the API (default) or the CLI as the main class. This allows you to run the CLI via
In my testing, |
… `logback-spring.xml` when running as a service Updating `pom.xml` to use a specific file name and set the logback-cli.xml file as the primary for the `cli` profile in maven
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (4)
Java/measureeval/src/main/resources/logback-cli.xml (1)
15-17: Consider adjusting the log level for org.opencds.cqf.The logger for the
org.opencds.cqfpackage is set to the "debug" level, which may generate verbose logs. While this could be intentional for detailed debugging of the CQF library, it might be too verbose for regular CLI usage.Consider adjusting the log level to "info" for regular usage:
- <logger name="org.opencds.cqf" level="debug" additivity="false"> + <logger name="org.opencds.cqf" level="info" additivity="false"> <appender-ref ref="CONSOLE"/> </logger>Alternatively, you could make this configurable via a command-line parameter to allow users to enable debug logging when needed.
Java/measureeval/pom.xml (3)
161-170: LGTM: 'api' profile addition with a minor suggestion.The 'api' profile is well-structured and correctly set as the default. This ensures backward compatibility with existing builds.
Consider adding a brief comment above the profile to explain its purpose, e.g.:
<!-- Profile for running the application as a Spring Boot API --> <profile> <id>api</id> ... </profile>This would enhance the readability and maintainability of the POM file.
172-181: LGTM: 'cli' profile addition with a minor suggestion.The 'cli' profile is well-structured and correctly implements the CLI functionality as per the PR objectives. The custom logback configuration and final name are good additions.
Similar to the 'api' profile, consider adding a brief comment above the profile to explain its purpose, e.g.:
<!-- Profile for building the application as a command-line interface tool --> <profile> <id>cli</id> ... </profile>This would enhance the readability and maintainability of the POM file.
Line range hint
149-182: Overall changes look good. Consider updating documentation.The introduction of 'api' and 'cli' profiles effectively implements the new CLI functionality while maintaining the existing API mode. This approach aligns well with the PR objectives and provides a clean separation of concerns.
To ensure proper usage of these new profiles, consider the following suggestions:
- Update the project's README or documentation to explain how to build and run the application in both API and CLI modes.
- Provide examples of Maven commands to activate each profile, e.g.:
mvn clean package -Papi mvn clean package -Pcli- Document the differences in the resulting JAR files and how to execute them, especially for the CLI mode.
These documentation updates will help users and other developers understand and utilize the new functionality effectively.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- Java/measureeval/pom.xml (2 hunks)
- Java/measureeval/src/main/resources/logback-cli.xml (1 hunks)
Additional comments not posted (4)
Java/measureeval/src/main/resources/logback-cli.xml (3)
3-9: LGTM: Console appender configuration is well-structured.The console appender is correctly configured with a comprehensive log pattern that includes timestamp, thread name, log level, logger name, and message. This setup provides detailed information for effective debugging and log analysis.
11-13: LGTM: Logger configuration for com.lantanagroup.link is appropriate.The logger for the
com.lantanagroup.linkpackage is correctly set to the "info" level, which is suitable for general application logging. Setting additivity to false prevents log duplication, ensuring clean log output. This configuration aligns well with the PR objectives for CLI logging.
19-21: LGTM: Root logger configuration is appropriate.The root logger is correctly set to the "error" level, which is a good practice for CLI applications. This configuration ensures that only critical issues are logged by default, minimizing noise from third-party libraries while still capturing important error messages.
Java/measureeval/pom.xml (1)
149-149: LGTM: Dynamic main class configuration.The addition of
<mainClass>${mainClass}</mainClass>allows for flexible specification of the main class. This is a good practice, enabling different entry points based on profiles or build configurations.
Adding FileSystemInvocation class that allows evaluating a measure using a data from the locally running file system
Format:
java -jar measureeval.jar -cp com.lantanagroup.link.measureeval.FileSystemInvocation "<measure-bundle-path>" "<patient-bundle-path>" "<start>" "<end>"Example:
java -jar measureeval.jar -cp com.lantanagroup.link.measureeval.FileSystemInvocation "C:/path/to/measure-bundle.json" "C:/path/to/patient-bundle.json" "2021-01-01" "2021-12-31"Or directly via IntelliJ with a debug configuration.
Summary by CodeRabbit
New Features
Bug Fixes