Iris: Track token usage of iris requests
#9455
Conversation
github-actions
bot
added
tests
server
alexjoham
changed the title
alexjoham
changed the title
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsage.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsage.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMServiceType.java
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
WalkthroughThe enumeration Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 25
🧹 Outside diff range comments (2)
src/main/java/de/tum/cit/aet/artemis/iris/dto/IrisChatWebsocketDTO.java (1)
Line range hint
62-65: Updateequalsmethod to include the newtokensfield.The
equalsmethod has not been updated to include the newtokensfield. This could lead to inconsistent behavior when comparingIrisChatWebsocketDTOobjects.Please update the
equalsmethod to include thetokensfield in the comparison. Here's a suggested implementation:@Override public boolean equals(Object o) { if (this == o) { return true; } if (o == null || getClass() != o.getClass()) { return false; } IrisChatWebsocketDTO that = (IrisChatWebsocketDTO) o; return type == that.type && Objects.equals(message, that.message) && Objects.equals(rateLimitInfo, that.rateLimitInfo) && Objects.equals(stages, that.stages) && Objects.equals(suggestions, that.suggestions) && Objects.equals(tokens, that.tokens); }Also, consider overriding the
hashCodemethod to maintain consistency with the updatedequalsmethod.src/test/java/de/tum/cit/aet/artemis/iris/IrisCompetencyGenerationIntegrationTest.java (1)
Line range hint
37-85: Test method adheres well to coding guidelines. Consider minor enhancements.The test method
generateCompetencies_asEditor_shouldSucceedgenerally follows the provided coding guidelines:
- Descriptive naming
- Small, specific test case
- Use of fixed data
- JUnit 5 features
- assertThat for assertions
- Avoidance of direct DB access
- Use of mocking
Consider the following enhancements:
- Add explicit DB query count tracking if relevant to this test case.
- Ensure ArchUnit is used elsewhere to enforce package rules if not applicable here.
- If possible, add a comment or mechanism to address context restart minimization.
These suggestions aim to fully align the test with all aspects of the provided coding guidelines.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (2)
src/main/resources/config/liquibase/changelog/20241012125003_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (16)
- src/main/java/de/tum/cit/aet/artemis/core/domain/LLMServiceType.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsage.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/core/repository/LLMTokenUsageRepository.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/core/service/LLMTokenUsageService.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/dto/IrisChatWebsocketDTO.java (3 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.java (4 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatStatusUpdateDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyStatusUpdateDTO.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLLMCostDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (4 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (4 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/websocket/IrisChatWebsocketService.java (4 hunks)
- src/main/webapp/app/entities/iris/iris-message.model.ts (2 hunks)
- src/test/java/de/tum/cit/aet/artemis/iris/IrisChatMessageIntegrationTest.java (1 hunks)
- src/test/java/de/tum/cit/aet/artemis/iris/IrisChatWebsocketTest.java (1 hunks)
- src/test/java/de/tum/cit/aet/artemis/iris/IrisCompetencyGenerationIntegrationTest.java (1 hunks)
🧰 Additional context used
📓 Path-based instructions (16)
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMServiceType.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsage.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/core/repository/LLMTokenUsageRepository.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/core/service/LLMTokenUsageService.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/iris/dto/IrisChatWebsocketDTO.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatStatusUpdateDTO.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyStatusUpdateDTO.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLLMCostDTO.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/websocket/IrisChatWebsocketService.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/webapp/app/entities/iris/iris-message.model.ts (1)
src/test/java/de/tum/cit/aet/artemis/iris/IrisChatMessageIntegrationTest.java (1)
Pattern
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: truesrc/test/java/de/tum/cit/aet/artemis/iris/IrisChatWebsocketTest.java (1)
Pattern
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: truesrc/test/java/de/tum/cit/aet/artemis/iris/IrisCompetencyGenerationIntegrationTest.java (1)
Pattern
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
📓 Learnings (2)
src/main/java/de/tum/cit/aet/artemis/iris/service/websocket/IrisChatWebsocketService.java (2)
Learnt from: Hialus PR: ls1intum/Artemis#8286 File: src/main/java/de/tum/in/www1/artemis/service/iris/websocket/IrisChatWebsocketService.java:48-0 Timestamp: 2024-10-08T15:35:42.972Z Learning: In the `IrisChatWebsocketService.java` file, the `sendMessage` method is designed to allow exceptions to propagate without specific handling, as this is an intentional part of the error handling strategy.Learnt from: Hialus PR: ls1intum/Artemis#8286 File: src/main/java/de/tum/in/www1/artemis/service/iris/websocket/IrisChatWebsocketService.java:48-0 Timestamp: 2024-10-08T15:35:52.595Z Learning: In the `IrisChatWebsocketService.java` file, the `sendMessage` method is designed to allow exceptions to propagate without specific handling, as this is an intentional part of the error handling strategy.src/test/java/de/tum/cit/aet/artemis/iris/IrisCompetencyGenerationIntegrationTest.java (2)
Learnt from: julian-christl PR: ls1intum/Artemis#8052 File: src/test/java/de/tum/in/www1/artemis/lecture/CompetencyIntegrationTest.java:310-310 Timestamp: 2024-02-23T00:03:06.365Z Learning: Modifications to parameters in `competencyProgressUtilService.createCompetencyProgress` for debugging purposes are considered irrelevant to the test outcomes but helpful for clarity during debugging.Learnt from: julian-christl PR: ls1intum/Artemis#8052 File: src/test/java/de/tum/in/www1/artemis/lecture/CompetencyIntegrationTest.java:310-310 Timestamp: 2024-10-08T15:35:52.595Z Learning: Modifications to parameters in `competencyProgressUtilService.createCompetencyProgress` for debugging purposes are considered irrelevant to the test outcomes but helpful for clarity during debugging.
🔇 Additional comments (31)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLLMCostDTO.java (3)
1-1: LGTM: Package declaration is correct.The package declaration follows the standard Java naming conventions and aligns with the project structure.
3-3: LGTM: Import statement is appropriate.The import statement is specific and imports only the necessary
LLMServiceTypeenum, which is used in the record declaration. This adheres to the guideline of avoiding star imports.
1-6: Overall, well-structured DTO with minor naming convention issue.This new
PyrisLLMCostDTOrecord is well-designed and adheres to most of our coding guidelines:
- It follows the single responsibility principle.
- Uses Java records for DTOs as recommended.
- Promotes immutability.
- Uses primitive types where appropriate.
- Captures necessary information without redundancy.
The only improvement needed is to adjust the field names to follow Java's camelCase convention. Once this is addressed, the file will fully comply with our coding standards.
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMServiceType.java (2)
1-1: LGTM: Package declaration is correct.The package declaration follows the expected naming convention and structure.
4-5: Past review comment has been addressed.The distinction between
ATHENA_PRELIMINARY_FEEDBACKandATHENA_FEEDBACK_SUGGESTIONsuggested in the past review comment has been implemented in the current version of the enum.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatStatusUpdateDTO.java (2)
7-7: LGTM: New import added correctlyThe import for
PyrisLLMCostDTOis correctly added, which is necessary for the newtokensfield in the record. This change aligns with the PR objectives of tracking token usage.
11-11: LGTM: Record updated to include token usage trackingThe
PyrisChatStatusUpdateDTOrecord has been correctly updated to include the newtokensfield of typeList<PyrisLLMCostDTO>. This change:
- Aligns with the PR objectives of tracking token usage in Iris requests.
- Uses Java records, which is in line with the coding guidelines for DTOs.
- Follows the naming convention of CamelCase.
- Adheres to the principle of minimal DTOs by only adding the necessary field for token usage tracking.
The use of a List for
tokensallows for multiple token usage entries per chat status update, which provides flexibility in tracking token usage across different stages or components of a chat interaction.src/main/java/de/tum/cit/aet/artemis/core/repository/LLMTokenUsageRepository.java (5)
1-1: LGTM: Package declaration follows conventions.The package declaration adheres to Java naming conventions and follows the project's structure.
3-9: LGTM: Imports are well-organized and follow guidelines.The import statements are properly organized, grouped logically, and avoid wildcard imports as per the coding guidelines.
11-12: LGTM: Annotations are appropriate and align with project requirements.The
@Repositoryannotation correctly identifies this as a Spring Data repository, and the@Profile(PROFILE_IRIS)annotation ensures that this repository is only active when the Iris profile is in use, which aligns with the PR objectives.
13-14: LGTM: Interface declaration follows best practices and coding guidelines.The
LLMTokenUsageRepositoryinterface:
- Follows CamelCase naming convention.
- Extends the appropriate base repository (
ArtemisJpaRepository).- Adheres to the single responsibility principle.
- Correctly uses generics to specify the entity type (
LLMTokenUsage) and ID type (Long).The empty interface is appropriate as it inherits necessary methods from the base repository.
1-14: Overall: Excellent implementation of the LLMTokenUsageRepository.This new repository interface is well-designed and aligns perfectly with the PR objectives for tracking token usage in the Iris service. It adheres to all relevant coding guidelines, including:
- Proper package structure and naming conventions.
- Correct use of imports without wildcards.
- Appropriate use of Spring annotations (@repository and @Profile).
- Following the single responsibility principle and extending the correct base repository.
The empty interface is sufficient as it inherits all necessary methods from ArtemisJpaRepository. This implementation provides a solid foundation for managing LLMTokenUsage entities within the Artemis application.
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyStatusUpdateDTO.java (2)
7-7: LGTM: New import added for PyrisLLMCostDTOThe import for
PyrisLLMCostDTOis correctly added to support the new parameter in the record.
19-19: LGTM: Record updated to include token informationThe
PyrisCompetencyStatusUpdateDTOrecord has been correctly updated to include theList<PyrisLLMCostDTO> tokensparameter. This change aligns with the PR objectives to track token usage in Iris requests.A few observations:
- The naming convention follows CamelCase as required.
- The record adheres to the single responsibility principle, focusing on competency status updates.
- The use of
Listfor all parameters suggests consistency in data structure.- The record uses Java records, which aligns with the DTO guidelines.
src/main/webapp/app/entities/iris/iris-message.model.ts (1)
Line range hint
1-31: Overall structure and changes look goodThe overall structure of the file and the changes made align well with the PR objective of tracking token usage. The file adheres to most of the coding guidelines, including:
- Using single quotes for strings
- Using PascalCase for enums and types
- Following the Angular style guide
Once the property naming is adjusted to camelCase as suggested in the previous comments, the file will fully comply with the coding guidelines.
src/main/java/de/tum/cit/aet/artemis/iris/dto/IrisChatWebsocketDTO.java (3)
12-12: LGTM: Import statement added correctly.The new import for
PyrisLLMCostDTOis necessary for the addedtokensfield and follows the Java import guidelines.
25-26: LGTM: Record declaration updated appropriately.The addition of the
List<PyrisLLMCostDTO> tokensparameter to the record declaration is consistent with the PR objectives and follows coding guidelines. It maintains the single responsibility principle for this DTO.
Line range hint
1-73: Overall, the changes effectively implement token usage tracking.The modifications to
IrisChatWebsocketDTOsuccessfully incorporate the newtokensfield for tracking LLM token usage. The changes are consistent and adhere to coding guidelines. Minor improvements have been suggested for documentation and object comparison. Once these are addressed, the implementation will be robust and aligned with the PR objectives.src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (3)
22-22: LGTM: Import statement added correctly.The import for
LLMTokenUsageServiceis properly placed and follows the coding guideline of avoiding star imports.
47-48: LGTM: Field added correctly following best practices.The
llmTokenUsageServicefield is properly declared as private and final, adhering to the principle of least access. The use of constructor injection for this dependency is in line with best practices for dependency injection.
Line range hint
1-238: Consider adding documentation and improving error handling.The changes for incorporating token usage tracking have been implemented well and align with the PR objectives. To further improve the code:
Add JavaDoc comments to the new
llmTokenUsageServicefield and the modifiedhandleStatusUpdatemethod to explain their purpose and any important details about token usage tracking.Consider adding error handling for the token usage saving operations. For example:
try { llmTokenUsageService.saveTokenUsage(...); } catch (Exception e) { // Log the error and possibly notify an admin log.error("Failed to save token usage", e); }
- Ensure that the
LLMTokenUsageServiceis properly configured and tested. You might want to add integration tests for the token usage tracking functionality.To verify the proper configuration of
LLMTokenUsageService, please run the following command:This will help ensure that the new service is properly set up in the application context.
src/main/java/de/tum/cit/aet/artemis/core/service/LLMTokenUsageService.java (2)
63-64: Ensure handling whenuseris nullThe code sets
userIdonly ifuseris not null:if (user != null) { llmTokenUsage.setUserId(user.getId()); }Is it acceptable for
userIdto be null inLLMTokenUsage? IfuserIdis a non-nullable field in the database, this may cause issues.If
userIdmust not be null, consider throwing an exception or handling this case appropriately.
66-70: Check for potential null values inPyrisLLMCostDTOpropertiesWhen accessing properties of
PyrisLLMCostDTO, such asnum_input_tokens(),cost_per_input_token(), etc., there's no check for null values. If any of these methods can return null, it could lead toNullPointerException.Confirm whether the properties of
PyrisLLMCostDTOare guaranteed to be non-null. If they can be null, add checks or default values.src/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.java (3)
11-11: ImportLLMTokenUsageServiceadded correctlyThe import statement for
LLMTokenUsageServicehas been appropriately added, ensuring the class is recognized within this file.
29-29: Declaration ofllmTokenUsageServicealigns with best practicesThe
llmTokenUsageServiceis declared as aprivate finalfield, adhering to the principles of immutability and least access. This follows the best practice for dependency injection.
35-40: Constructor updated with new dependencyThe constructor has been correctly updated to include
LLMTokenUsageService llmTokenUsageService, and the assignmentthis.llmTokenUsageService = llmTokenUsageService;has been added. This aligns with the dependency injection via constructor injection guideline.src/main/java/de/tum/cit/aet/artemis/iris/service/websocket/IrisChatWebsocketService.java (5)
14-14: Addition ofPyrisLLMCostDTOimport is appropriateThe import of
PyrisLLMCostDTOis necessary for handling token cost data in the updated methods.
45-45: EnsureIrisChatWebsocketDTOhandlesnullvalues safelyWhen constructing
IrisChatWebsocketDTOwithnullvalues forsuggestionsandtokens, verify that the class handlesnullinputs appropriately to prevent potentialNullPointerExceptionissues.
56-56: Method overloading maintains backward compatibilityThe
sendStatusUpdatemethod now delegates to the overloaded method with additional parameters. This approach maintains backward compatibility and promotes code reuse.
70-70: Handle potentialnullvalues inpayloadWhen constructing the
payloadwithnullforirisMessage, ensure that theIrisChatWebsocketDTOclass and its consumers handlenullvalues correctly to prevent unexpected behaviors.
66-66:⚠️ Potential issueChanging public method signature may introduce breaking changes
The
sendStatusUpdatemethod signature has been updated to includesuggestionsandtokens. This could potentially break existing code that calls this method with the old signature. Ensure that all external callers have been updated to match the new method signature to prevent runtime errors.Run the following script to identify any usages of
sendStatusUpdatethat may need updating:✅ Verification successful
Verification Successful: No usages of
sendStatusUpdatewith the old signature found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to `sendStatusUpdate` with the old signature. # Test: Search for method calls to `sendStatusUpdate` with three arguments (excluding 'this' pointer). # Expect: No instances should be found; all calls should use the new signature with four arguments. ast-grep --lang java --pattern '$_.sendStatusUpdate($_, $_, $_)' --ignore 'IrisChatWebsocketService.java'Length of output: 326
Script:
#!/bin/bash # Description: Find all calls to `sendStatusUpdate` with the old signature, excluding IrisChatWebsocketService.java. # Using fd to list all Java files excluding IrisChatWebsocketService.java and passing them to ast-grep fd --extension java --exclude 'IrisChatWebsocketService.java' | xargs ast-grep --lang java --pattern '$_.sendStatusUpdate($_, $_, $_)'Length of output: 258
Script:
#!/bin/bash # Description: Find all calls to `sendStatusUpdate` with the old signature, excluding IrisChatWebsocketService.java and EncodingISO_8559_1.java. # Using fd to list all Java files excluding the specified problematic files and passing them to ast-grep fd --extension java --exclude 'IrisChatWebsocketService.java' --exclude 'EncodingISO_8559_1.java' | xargs ast-grep --lang java --pattern '$_.sendStatusUpdate($_, $_, $_)' 2>/dev/nullLength of output: 168
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLLMCostDTO.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMServiceType.java
Outdated
Show resolved
Hide resolved
...de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyStatusUpdateDTO.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsage.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsage.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsage.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java
Outdated
Show resolved
Hide resolved
alexjoham
changed the title
Iris: Track token usage of iris requests
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsage.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsage.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/service/LLMTokenUsageService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/service/LLMTokenUsageService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java
Outdated
Show resolved
Hide resolved
src/main/resources/config/liquibase/changelog/20241012125003_changelog.xml
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsage.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsage.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/service/LLMTokenUsageService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- src/main/java/de/tum/cit/aet/artemis/iris/service/websocket/IrisChatWebsocketService.java (4 hunks)
- src/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java (1 hunks)
- src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/main/java/de/tum/cit/aet/artemis/iris/service/websocket/IrisChatWebsocketService.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java (1)
Pattern
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: truesrc/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java (1)
Pattern
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
📓 Learnings (1)
src/main/java/de/tum/cit/aet/artemis/iris/service/websocket/IrisChatWebsocketService.java (2)
Learnt from: Hialus PR: ls1intum/Artemis#8286 File: src/main/java/de/tum/in/www1/artemis/service/iris/websocket/IrisChatWebsocketService.java:48-0 Timestamp: 2024-10-08T15:35:42.972Z Learning: In the `IrisChatWebsocketService.java` file, the `sendMessage` method is designed to allow exceptions to propagate without specific handling, as this is an intentional part of the error handling strategy.Learnt from: Hialus PR: ls1intum/Artemis#8286 File: src/main/java/de/tum/in/www1/artemis/service/iris/websocket/IrisChatWebsocketService.java:48-0 Timestamp: 2024-10-08T15:35:52.595Z Learning: In the `IrisChatWebsocketService.java` file, the `sendMessage` method is designed to allow exceptions to propagate without specific handling, as this is an intentional part of the error handling strategy.
🔇 Additional comments (5)
src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java (1)
Line range hint
1-401: Overall, the test file maintains good quality and adheres to guidelinesThe
IrisTextExerciseChatMessageIntegrationTestclass follows the prescribed coding guidelines for test files. It uses descriptive test names, keeps tests small and specific, utilizes JUnit 5 features, and employs AssertThat assertions. The tests avoid direct database access and use mocks appropriately.The minimal change to accommodate the new feature is correctly implemented. However, to fully leverage this change, consider expanding the test coverage as suggested in the previous comment.
src/main/java/de/tum/cit/aet/artemis/iris/service/websocket/IrisChatWebsocketService.java (1)
14-14: Approved: Necessary import addedThe import statement for
PyrisLLMCostDTOis appropriately added to support the new token usage tracking functionality.src/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java (3)
253-256: Good use of fixed data in test methodsThe
createMockTextContent()method uses fixed, deterministic data, which aligns with the coding guideline (fixed_data: true). This approach ensures that tests are predictable and reproducible.
162-165: Appropriate use ofassertThatfor specific assertionsThe assertions in the loop are specific and use
assertThat, complying with the coding guidelines (assert_use: assertThat,assert_specificity: true). This enhances the readability and consistency of the tests.
64-65: Use of@SpyBeanshould be reconsideredAs previously noted, according to the coding guidelines:
Do not use the
@SpyBeanor@MockBeanannotation unless absolutely necessary or possibly in an abstract Superclass.Since
@SpyBeancan introduce complexity and might lead to unintended interactions with the application context, consider using static mocks or refactoring your code to achieve the desired test coverage without it.
src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java
Show resolved
Hide resolved
| var user = session.getUser(); | ||
| var rateLimitInfo = rateLimitService.getRateLimitInformation(user); | ||
| var topic = "" + session.getId(); // Todo: add more specific topic | ||
| var payload = new IrisChatWebsocketDTO(null, rateLimitInfo, stages, suggestions); | ||
| var payload = new IrisChatWebsocketDTO(null, rateLimitInfo, stages, suggestions, tokens); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid passing multiple null parameters to constructor
Passing null values for optional parameters in the IrisChatWebsocketDTO constructor can be improved for better code clarity. Consider using a builder pattern or method overloading to avoid passing null.
Refactor the payload creation as follows:
-var payload = new IrisChatWebsocketDTO(null, rateLimitInfo, stages, suggestions, tokens);
+var payload = new IrisChatWebsocketDTO(rateLimitInfo, stages, suggestions, tokens);And modify the constructor accordingly to handle cases where irisMessage is not provided.
Alternatively, if a builder pattern is available:
var payload = IrisChatWebsocketDTO.builder()
.withRateLimitInfo(rateLimitInfo)
.withStages(stages)
.withSuggestions(suggestions)
.withTokens(tokens)
.build();| */ | ||
| public void sendStatusUpdate(IrisChatSession session, List<PyrisStageDTO> stages, List<String> suggestions) { | ||
| public void sendStatusUpdate(IrisChatSession session, List<PyrisStageDTO> stages, List<String> suggestions, List<PyrisLLMCostDTO> tokens) { |
There was a problem hiding this comment.
💡 Codebase verification
Unupdated Callers of sendStatusUpdate Identified
src/main/java/de/tum/cit/aet/artemis/iris/service/websocket/IrisChatWebsocketService.java:sendStatusUpdate(session, stages)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisPipelineService.java:irisChatWebsocketService.sendStatusUpdate(session, stages)
🔗 Analysis chain
Ensure consistency with method signature changes
The sendStatusUpdate method now includes additional parameters List<String> suggestions and List<PyrisLLMCostDTO> tokens. Verify that all callers of this method are updated accordingly to prevent compile-time errors or unexpected behavior.
Run the following script to identify all usages of sendStatusUpdate:
This will help you locate and update all method calls to match the new signature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of 'sendStatusUpdate' to ensure they match the new signature.
rg --type java 'sendStatusUpdate\('
Length of output: 1395
| @BeforeEach | ||
| void initTestCase() throws GitAPIException, IOException, URISyntaxException { | ||
| userUtilService.addUsers(TEST_PREFIX, 2, 0, 0, 0); | ||
|
|
||
| course = programmingExerciseUtilService.addCourseWithOneProgrammingExercise(); | ||
| exercise = exerciseUtilService.getFirstExerciseWithType(course, ProgrammingExercise.class); | ||
| String projectKey = exercise.getProjectKey(); | ||
| exercise.setProjectType(ProjectType.PLAIN_GRADLE); | ||
| exercise.setTestRepositoryUri(localVCBaseUrl + "/git/" + projectKey + "/" + projectKey.toLowerCase() + "-tests.git"); | ||
| programmingExerciseBuildConfigRepository.save(exercise.getBuildConfig()); | ||
| programmingExerciseRepository.save(exercise); | ||
| exercise = programmingExerciseRepository.findWithAllParticipationsAndBuildConfigById(exercise.getId()).orElseThrow(); | ||
|
|
||
| // Set the correct repository URIs for the template and the solution participation. | ||
| String templateRepositorySlug = projectKey.toLowerCase() + "-exercise"; | ||
| TemplateProgrammingExerciseParticipation templateParticipation = exercise.getTemplateParticipation(); | ||
| templateParticipation.setRepositoryUri(localVCBaseUrl + "/git/" + projectKey + "/" + templateRepositorySlug + ".git"); | ||
| templateProgrammingExerciseParticipationRepository.save(templateParticipation); | ||
| String solutionRepositorySlug = projectKey.toLowerCase() + "-solution"; | ||
| SolutionProgrammingExerciseParticipation solutionParticipation = exercise.getSolutionParticipation(); | ||
| solutionParticipation.setRepositoryUri(localVCBaseUrl + "/git/" + projectKey + "/" + solutionRepositorySlug + ".git"); | ||
| solutionProgrammingExerciseParticipationRepository.save(solutionParticipation); | ||
|
|
||
| String assignmentRepositorySlug = projectKey.toLowerCase() + "-" + TEST_PREFIX + "student1"; | ||
|
|
||
| // Add a participation for student1. | ||
| ProgrammingExerciseStudentParticipation studentParticipation = participationUtilService.addStudentParticipationForProgrammingExercise(exercise, TEST_PREFIX + "student1"); | ||
| studentParticipation.setRepositoryUri(String.format(localVCBaseUrl + "/git/%s/%s.git", projectKey, assignmentRepositorySlug)); | ||
| studentParticipation.setBranch(defaultBranch); | ||
| programmingExerciseStudentParticipationRepository.save(studentParticipation); | ||
|
|
||
| // Prepare the repositories. | ||
| localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, templateRepositorySlug); | ||
| localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, projectKey.toLowerCase() + "-tests"); | ||
| localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, solutionRepositorySlug); | ||
| localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, assignmentRepositorySlug); | ||
|
|
||
| // Check that the repository folders were created in the file system for all base repositories. | ||
| localVCLocalCITestService.verifyRepositoryFoldersExist(exercise, localVCBasePath); | ||
|
|
||
| activateIrisGlobally(); | ||
| activateIrisFor(course); | ||
| activateIrisFor(exercise); | ||
| pipelineDone = new AtomicBoolean(false); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing test setup to minimize context restarts
The initTestCase() method annotated with @BeforeEach is performing extensive setup that might not need to be re-run before each individual test. According to the coding guidelines (context_restart_minimize: true), minimizing context restarts can significantly improve test performance.
If the setup is static and doesn't change between tests, consider moving it to a method annotated with @BeforeAll, which runs once before all tests. Remember that @BeforeAll methods need to be static, or you can use the @TestInstance(Lifecycle.PER_CLASS) annotation to use non-static methods.
|
|
||
| var irisSession = irisExerciseChatSessionService.createChatSessionForProgrammingExercise(exercise, userUtilService.getUserByLogin(TEST_PREFIX + "student1")); | ||
| var irisMessage = createDefaultMockMessage(irisSession); | ||
| irisMessageRepository.save(irisMessage); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid direct database access in tests
The line irisMessageRepository.save(irisMessage); involves direct database interaction. According to the coding guidelines (avoid_db_access: true), tests should avoid direct database access to improve performance and reduce flakiness.
Consider mocking the IrisMessageRepository or using a service layer that abstracts away the database interactions. This approach allows you to test the functionality without relying on the database.
| private List<PyrisLLMCostDTO> getMockLLMCosts() { | ||
| List<PyrisLLMCostDTO> costs = new ArrayList<>(); | ||
| for (int i = 0; i < 5; i++) { | ||
| costs.add(new PyrisLLMCostDTO("test-llm", i * 10 + 5, i * 0.5f, i * 3 + 5, i * 0.12f, LLMServiceType.IRIS_CHAT_EXERCISE_MESSAGE)); | ||
| } | ||
| return costs; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parameterize tests to improve maintainability and readability
The method getMockLLMCosts() generates a list of token costs used across multiple tests. According to the coding guidelines (test_size: small_specific), tests should be small and focused. Using JUnit 5's @ParameterizedTest can help you run the same test logic with different inputs, reducing code duplication and improving test coverage.
For example, you can refactor your tests to accept different PyrisLLMCostDTO inputs and validate the outcomes accordingly.
Make saveIrisTokenUsage saver
…ests' into feature/track-usage-of-iris-requests
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (2)
src/main/resources/config/liquibase/changelog/20241014035241_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (5)
- src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsage.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/core/service/LLMTokenUsageService.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.java (3 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLLMCostDTO.java (1 hunks)
- src/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsage.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/core/service/LLMTokenUsageService.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLLMCostDTO.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java (1)
Pattern
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
🔇 Additional comments (2)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/data/PyrisLLMCostDTO.java (2)
1-1: LGTM: Package declaration is correct.The package declaration follows the correct naming convention for Java packages.
3-4: LGTM: Well-structured and improved record declaration.The
PyrisLLMCostDTOrecord is well-designed and addresses previous review comments:
- Adheres to the single responsibility principle, focusing on LLM cost data.
- Uses appropriate primitive types (int for token counts, float for costs).
- Promotes immutability through the use of a record, which is ideal for DTOs.
- Captures necessary information without redundancy.
- Follows Java naming conventions with camelCase field names.
- Uses String for the
pipelinefield, allowing flexibility for unknown service types.These changes improve the overall design and address the concerns raised in previous reviews.
| Course course = courseRepository.findByIdForUpdateElseThrow(job.courseId()); | ||
| if (statusUpdate.tokens() != null) { | ||
| llmTokenUsageService.saveIrisTokenUsage(job, course, statusUpdate.tokens()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize database call by moving 'course' retrieval inside the conditional block
The course object is only utilized when statusUpdate.tokens() is not null. To improve performance by avoiding unnecessary database queries when tokens are absent, consider moving the retrieval of the course inside the if block.
Apply this diff to implement the change:
public void handleStatusUpdate(CompetencyExtractionJob job, PyrisCompetencyStatusUpdateDTO statusUpdate) {
- Course course = courseRepository.findByIdForUpdateElseThrow(job.courseId());
if (statusUpdate.tokens() != null) {
+ Course course = courseRepository.findByIdForUpdateElseThrow(job.courseId());
llmTokenUsageService.saveIrisTokenUsage(job, course, statusUpdate.tokens());
}
websocketService.send(job.userLogin(), websocketTopic(job.courseId()), statusUpdate);
}📝 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.
| Course course = courseRepository.findByIdForUpdateElseThrow(job.courseId()); | |
| if (statusUpdate.tokens() != null) { | |
| llmTokenUsageService.saveIrisTokenUsage(job, course, statusUpdate.tokens()); | |
| } | |
| if (statusUpdate.tokens() != null) { | |
| Course course = courseRepository.findByIdForUpdateElseThrow(job.courseId()); | |
| llmTokenUsageService.saveIrisTokenUsage(job, course, statusUpdate.tokens()); | |
| } |
| @Table(name = "llm_token_usage") | ||
| @Cache(usage = CacheConcurrencyStrategy.NONSTRICT_READ_WRITE) | ||
| @JsonInclude(JsonInclude.Include.NON_EMPTY) | ||
| public class LLMTokenUsage extends DomainObject { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding JavaDoc comments for the class
Adding a class-level JavaDoc comment can improve code readability and help other developers understand the purpose and usage of this entity.
| @Column(name = "time") | ||
| private ZonedDateTime time = ZonedDateTime.now(); |
There was a problem hiding this comment.
Avoid using reserved SQL keywords for column names
The column name time may conflict with reserved keywords in some SQL databases. To prevent potential issues, consider renaming the column to a non-reserved word like timestamp or escaping it using quotes in the @Column annotation.
Apply this change:
- @Column(name = "time")
+ @Column(name = "\"time\"")
private ZonedDateTime time = ZonedDateTime.now();Or rename the column:
- @Column(name = "time")
+ @Column(name = "timestamp")
private ZonedDateTime time = ZonedDateTime.now();Ensure that any database scripts or queries are updated to reflect this change.
📝 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.
| @Column(name = "time") | |
| private ZonedDateTime time = ZonedDateTime.now(); | |
| @Column(name = "\"time\"") | |
| private ZonedDateTime time = ZonedDateTime.now(); |
| } | ||
| llmTokenUsage.setServiceType(cost.pipeline()); | ||
| llmTokenUsage.setExercise(exercise); | ||
| llmTokenUsage.setCourse(course); |
There was a problem hiding this comment.
Add null check before setting course to prevent potential NullPointerException
The course parameter may be null in some cases, as seen in the overloaded methods where course could be omitted. To ensure robustness, add a null check before setting the course in llmTokenUsage.
Apply this diff to add the null check:
llmTokenUsage.setServiceType(cost.pipeline());
-llmTokenUsage.setCourse(course);
+if (course != null) {
+ llmTokenUsage.setCourse(course);
+}
llmTokenUsage.setNumInputTokens(cost.numInputTokens());📝 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.
| llmTokenUsage.setCourse(course); | |
| llmTokenUsage.setServiceType(cost.pipeline()); | |
| if (course != null) { | |
| llmTokenUsage.setCourse(course); | |
| } | |
| llmTokenUsage.setNumInputTokens(cost.numInputTokens()); |
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") | ||
| void testTokenTrackingHandledExerciseChat() throws Exception { | ||
| var irisSession = irisExerciseChatSessionService.createChatSessionForProgrammingExercise(exercise, userUtilService.getUserByLogin(TEST_PREFIX + "student1")); | ||
| var messageToSend = createDefaultMockMessage(irisSession); | ||
|
|
||
| var tokens = getMockLLMCosts(); | ||
|
|
||
| List<PyrisStageDTO> doneStage = new ArrayList<>(); | ||
| doneStage.add(new PyrisStageDTO("DoneTest", 10, PyrisStageState.DONE, "Done")); | ||
|
|
||
| irisRequestMockProvider.mockProgrammingExerciseChatResponse(dto -> { | ||
| assertThat(dto.settings().authenticationToken()).isNotNull(); | ||
|
|
||
| assertThatNoException().isThrownBy(() -> sendStatus(dto.settings().authenticationToken(), "Hello World", doneStage, null, tokens)); | ||
|
|
||
| pipelineDone.set(true); | ||
| }); | ||
|
|
||
| request.postWithoutResponseBody("/api/iris/sessions/" + irisSession.getId() + "/messages", messageToSend, HttpStatus.CREATED); | ||
|
|
||
| await().until(pipelineDone::get); | ||
|
|
||
| // Capture the saved token usages | ||
| ArgumentCaptor<List<PyrisLLMCostDTO>> captor = ArgumentCaptor.forClass(List.class); | ||
| verify(llmTokenUsageService).saveIrisTokenUsage(any(PyrisJob.class), any(IrisMessage.class), any(Exercise.class), any(User.class), any(Course.class), captor.capture()); | ||
|
|
||
| // Verify that the tokens are saved correctly | ||
| List<PyrisLLMCostDTO> savedTokenUsages = captor.getValue(); | ||
| assertThat(savedTokenUsages).hasSize(5); | ||
| for (int i = 0; i < savedTokenUsages.size(); i++) { | ||
| PyrisLLMCostDTO usage = savedTokenUsages.get(i); | ||
| PyrisLLMCostDTO expectedCost = tokens.get(i); | ||
|
|
||
| assertThat(usage.numInputTokens()).isEqualTo(expectedCost.numInputTokens()); | ||
| assertThat(usage.costPerInputToken()).isEqualTo(expectedCost.costPerInputToken()); | ||
| assertThat(usage.numOutputTokens()).isEqualTo(expectedCost.numOutputTokens()); | ||
| assertThat(usage.costPerOutputToken()).isEqualTo(expectedCost.costPerOutputToken()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Common Test Logic to Reduce Duplication
The methods testTokenTrackingHandledExerciseChat() and testTokenTrackingExerciseChatWithPipelineFail() contain duplicated code for setting up sessions, sending messages, and verifying token usages. Refactoring the common logic into helper methods will adhere to the DRY (Don't Repeat Yourself) principle and enhance maintainability.
Apply this diff to extract common code:
+ private void executeTokenTrackingTest(IrisSession irisSession, IrisMessage messageToSend, List<PyrisStageDTO> stages, List<PyrisLLMCostDTO> tokens, boolean expectError) throws Exception {
+ irisRequestMockProvider.mockProgrammingExerciseChatResponse(dto -> {
+ assertThat(dto.settings().authenticationToken()).isNotNull();
+ assertThatNoException().isThrownBy(() -> sendStatus(dto.settings().authenticationToken(), expectError ? null : "Hello World", stages, null, tokens));
+ pipelineDone.set(true);
+ });
+ request.postWithoutResponseBody("/api/iris/sessions/" + irisSession.getId() + "/messages", messageToSend, HttpStatus.CREATED);
+ await().until(pipelineDone::get);
+ ArgumentCaptor<List<PyrisLLMCostDTO>> captor = ArgumentCaptor.forClass(List.class);
+ verify(llmTokenUsageService).saveIrisTokenUsage(any(PyrisJob.class), expectError ? isNull() : any(IrisMessage.class), any(Exercise.class), any(User.class), any(Course.class), captor.capture());
+ List<PyrisLLMCostDTO> savedTokenUsages = captor.getValue();
+ assertThat(savedTokenUsages).hasSize(5);
+ for (int i = 0; i < savedTokenUsages.size(); i++) {
+ PyrisLLMCostDTO usage = savedTokenUsages.get(i);
+ PyrisLLMCostDTO expectedCost = tokens.get(i);
+ assertThat(usage.numInputTokens()).isEqualTo(expectedCost.numInputTokens());
+ assertThat(usage.costPerInputToken()).isEqualTo(expectedCost.costPerInputToken());
+ assertThat(usage.numOutputTokens()).isEqualTo(expectedCost.numOutputTokens());
+ assertThat(usage.costPerOutputToken()).isEqualTo(expectedCost.costPerOutputToken());
+ }
+ }Then, update the test methods:
void testTokenTrackingHandledExerciseChat() throws Exception {
var irisSession = irisExerciseChatSessionService.createChatSessionForProgrammingExercise(exercise, userUtilService.getUserByLogin(TEST_PREFIX + "student1"));
var messageToSend = createDefaultMockMessage(irisSession);
var tokens = getMockLLMCosts();
List<PyrisStageDTO> doneStage = new ArrayList<>();
doneStage.add(new PyrisStageDTO("DoneTest", 10, PyrisStageState.DONE, "Done"));
- // Existing test logic...
+ executeTokenTrackingTest(irisSession, messageToSend, doneStage, tokens, false);
}void testTokenTrackingExerciseChatWithPipelineFail() throws Exception {
var irisSession = irisExerciseChatSessionService.createChatSessionForProgrammingExercise(exercise, userUtilService.getUserByLogin(TEST_PREFIX + "student1"));
var messageToSend = createDefaultMockMessage(irisSession);
var tokens = getMockLLMCosts();
List<PyrisStageDTO> failedStages = new ArrayList<>();
failedStages.add(new PyrisStageDTO("TestTokenFail", 10, PyrisStageState.ERROR, "Failed running pipeline"));
- // Existing test logic...
+ executeTokenTrackingTest(irisSession, messageToSend, failedStages, tokens, true);
}Also applies to: 198-237
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java (1)
Pattern
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
| import de.tum.cit.aet.artemis.programming.domain.SolutionProgrammingExerciseParticipation; | ||
| import de.tum.cit.aet.artemis.programming.domain.TemplateProgrammingExerciseParticipation; | ||
|
|
||
| class IrisChatTokenTrackingIntegrationTest extends AbstractIrisIntegrationTest { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add JavaDoc comments to the test class
Adding JavaDoc comments to the IrisChatTokenTrackingIntegrationTest class will improve readability and help future developers understand the purpose and functionality of the tests.
You can add a class-level JavaDoc like:
/**
* Integration tests for tracking LLM token usage in Iris chat sessions.
*/
class IrisChatTokenTrackingIntegrationTest extends AbstractIrisIntegrationTest {
// ...
}| assertThatNoException().isThrownBy(() -> sendStatus(dto.settings().authenticationToken(), "Hello World", doneStage, null, tokens)); | ||
|
|
There was a problem hiding this comment.
Specify timeout for Awaitility to prevent indefinite waits
In the test method testTokenTrackingHandledExerciseChat, the use of await().until(pipelineDone::get); without a timeout could lead to tests hanging indefinitely if the condition is never met. According to best practices, it's advisable to specify a maximum timeout to ensure the test fails within a reasonable time when the condition isn't satisfied.
Apply this change to set a timeout:
- await().until(pipelineDone::get);
+ await().atMost(Duration.ofSeconds(10)).until(pipelineDone::get);Don't forget to import java.time.Duration.
Committable suggestion was skipped due to low confidence.
|
|
||
| request.postWithoutResponseBody("/api/iris/sessions/" + irisSession.getId() + "/messages", messageToSend, HttpStatus.CREATED); | ||
|
|
||
| await().until(pipelineDone::get); |
There was a problem hiding this comment.
Specify timeout for Awaitility to prevent indefinite waits
In the test method testTokenTrackingExerciseChatWithPipelineFail, using await().until(pipelineDone::get); without specifying a timeout may cause the test to hang indefinitely if the condition isn't met. To prevent this, set a maximum timeout to ensure the test terminates appropriately.
Apply this change to include a timeout:
- await().until(pipelineDone::get);
+ await().atMost(Duration.ofSeconds(10)).until(pipelineDone::get);Ensure you have imported java.time.Duration.
📝 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.
| await().until(pipelineDone::get); | |
| await().atMost(Duration.ofSeconds(10)).until(pipelineDone::get); |
| List<PyrisLLMCostDTO> costs = new ArrayList<>(); | ||
| for (int i = 0; i < 5; i++) { | ||
| costs.add(new PyrisLLMCostDTO("test-llm", i * 10 + 5, i * 0.5f, i * 3 + 5, i * 0.12f, LLMServiceType.IRIS_CHAT_EXERCISE_MESSAGE.toString())); | ||
| } | ||
| return costs; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify getMockLLMCosts method using Java Streams
The getMockLLMCosts() method uses a loop to populate a list, which can be simplified using Java Streams for better readability and conciseness.
Apply this change:
private List<PyrisLLMCostDTO> getMockLLMCosts() {
- List<PyrisLLMCostDTO> costs = new ArrayList<>();
- for (int i = 0; i < 5; i++) {
- costs.add(new PyrisLLMCostDTO("test-llm", i * 10 + 5, i * 0.5f, i * 3 + 5, i * 0.12f, LLMServiceType.IRIS_CHAT_EXERCISE_MESSAGE.toString()));
- }
- return costs;
+ return IntStream.range(0, 5)
+ .mapToObj(i -> new PyrisLLMCostDTO("test-llm", i * 10 + 5, i * 0.5f, i * 3 + 5, i * 0.12f, LLMServiceType.IRIS_CHAT_EXERCISE_MESSAGE.toString()))
+ .collect(Collectors.toList());
}Don't forget to import java.util.stream.Collectors and java.util.stream.IntStream.
📝 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.
| List<PyrisLLMCostDTO> costs = new ArrayList<>(); | |
| for (int i = 0; i < 5; i++) { | |
| costs.add(new PyrisLLMCostDTO("test-llm", i * 10 + 5, i * 0.5f, i * 3 + 5, i * 0.12f, LLMServiceType.IRIS_CHAT_EXERCISE_MESSAGE.toString())); | |
| } | |
| return costs; | |
| } | |
| private List<PyrisLLMCostDTO> getMockLLMCosts() { | |
| return IntStream.range(0, 5) | |
| .mapToObj(i -> new PyrisLLMCostDTO("test-llm", i * 10 + 5, i * 0.5f, i * 3 + 5, i * 0.12f, LLMServiceType.IRIS_CHAT_EXERCISE_MESSAGE.toString())) | |
| .collect(Collectors.toList()); | |
| } |
|
github-actions
bot
added
the
deployment-error
alexjoham
added
deploy:artemis-test3
and removed
deployment-error
alexjoham
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 14
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (2)
src/main/resources/config/liquibase/changelog/20241015043720_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (5)
- src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsage.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/core/service/LLMTokenUsageService.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.java (3 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/job/CompetencyExtractionJob.java (1 hunks)
- src/test/java/de/tum/cit/aet/artemis/iris/IrisCompetencyGenerationIntegrationTest.java (2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsage.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/core/service/LLMTokenUsageService.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/job/CompetencyExtractionJob.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/test/java/de/tum/cit/aet/artemis/iris/IrisCompetencyGenerationIntegrationTest.java (1)
Pattern
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
📓 Learnings (1)
src/test/java/de/tum/cit/aet/artemis/iris/IrisCompetencyGenerationIntegrationTest.java (2)
Learnt from: julian-christl PR: ls1intum/Artemis#8052 File: src/test/java/de/tum/in/www1/artemis/lecture/CompetencyIntegrationTest.java:310-310 Timestamp: 2024-02-23T00:03:06.365Z Learning: Modifications to parameters in `competencyProgressUtilService.createCompetencyProgress` for debugging purposes are considered irrelevant to the test outcomes but helpful for clarity during debugging.Learnt from: julian-christl PR: ls1intum/Artemis#8052 File: src/test/java/de/tum/in/www1/artemis/lecture/CompetencyIntegrationTest.java:310-310 Timestamp: 2024-10-08T15:35:52.595Z Learning: Modifications to parameters in `competencyProgressUtilService.createCompetencyProgress` for debugging purposes are considered irrelevant to the test outcomes but helpful for clarity during debugging.
🔇 Additional comments (3)
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/job/CompetencyExtractionJob.java (2)
6-6: LGTM: Import addition is appropriate.The addition of the
Userimport is necessary for the updated record parameter and follows the coding guideline to avoid star imports.
Line range hint
1-24: Overall, the changes improve the design and maintain good coding practices.The modification from
String userLogintoUser userenhances the record's structure and type safety. The file continues to adhere to important coding principles such as:
- Single Responsibility Principle
- Constructor Injection for dependency management
- Principle of Least Access in the
canAccessmethodThese changes lay a good foundation for potentially expanded user-related functionality in the future while maintaining a clean and maintainable codebase.
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsage.java (1)
50-51:⚠️ Potential issueAvoid Using Reserved SQL Keywords for Column Names
The column name
timemay conflict with reserved keywords in some SQL databases. To prevent potential issues, consider renaming the column totimestampor escaping it.Apply this change:
- @Column(name = "time") + @Column(name = "\"time\"") private ZonedDateTime time = ZonedDateTime.now();Or rename the column:
- @Column(name = "time") + @Column(name = "timestamp") private ZonedDateTime time = ZonedDateTime.now();Ensure that any database scripts or queries are updated to reflect this change.
Likely invalid or redundant comment.
| * @param user the user who started the job | ||
| */ | ||
| @JsonInclude(JsonInclude.Include.NON_EMPTY) | ||
| public record CompetencyExtractionJob(String jobId, long courseId, String userLogin) implements PyrisJob { | ||
| public record CompetencyExtractionJob(String jobId, long courseId, User user) implements PyrisJob { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: Record update improves type safety.
The change from String userLogin to User user is a good improvement, providing more structure and potentially more functionality for user-related operations. This change aligns well with object-oriented principles and maintains the single responsibility of the record.
Minor suggestion: Consider expanding the JavaDoc for the user parameter to briefly explain its role or any significant properties it holds in the context of this job.
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsage.java
Outdated
Show resolved
Hide resolved
| @Column(name = "model") | ||
| private String model; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider Using an Enum for model Field
Similar to serviceType, the model field could benefit from being an enum to ensure only valid model names are used.
Apply these changes:
-
Define an
enumforLLMModel:package de.tum.cit.aet.artemis.core.domain; public enum LLMModel { GPT_3_5_TURBO, GPT_4, // Add other models as needed }
-
Update the
LLMTokenUsageclass:+ import jakarta.persistence.EnumType; + import jakarta.persistence.Enumerated; @Column(name = "model") + @Enumerated(EnumType.STRING) - private String model; + private LLMModel model;
-
Update the getter and setter methods:
- public String getModel() { + public LLMModel getModel() { return model; } - public void setModel(String model) { + public void setModel(LLMModel model) { this.model = model; }
| @Column(name = "user_id") | ||
| private long userId; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider Mapping userId as a @ManyToOne Relationship
Mapping the userId field as a @ManyToOne relationship to the User entity can enhance data integrity and simplify user-related queries.
Apply these changes:
-
Update the field declaration:
- @Column(name = "user_id") - private long userId; + @ManyToOne + @JoinColumn(name = "user_id") + private User user;
-
Update the getter and setter methods:
- public long getUserId() { - return userId; - } - public void setUserId(long userId) { - this.userId = userId; - } + public User getUser() { + return user; + } + public void setUser(User user) { + this.user = user; + }
-
Ensure that you import the necessary classes:
import jakarta.persistence.JoinColumn; import jakarta.persistence.ManyToOne; import de.tum.cit.aet.artemis.core.domain.User;
| @Override | ||
| public String toString() { | ||
| return "LLMTokenUsage{" + "serviceType=" + serviceType + ", model=" + model + ", numInputTokens=" + numInputTokens + ", costPerMillionInputTokens=" | ||
| + costPerMillionInputTokens + ", numOutputTokens=" + numOutputTokens + ", costPerMillionOutputTokens=" + costPerMillionOutputTokens + ", course=" + courseId | ||
| + ", exercise=" + exerciseId + ", userId=" + userId + ", timestamp=" + time + ", traceId=" + traceId + ", irisMessage=" + irisMessageId + '}'; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add Class-Level JavaDoc Comment
Adding a JavaDoc comment to the class LLMTokenUsage enhances code readability and provides clarity on the purpose and usage of the class.
Apply this change:
/**
* Entity representing the usage of tokens by Large Language Models (LLMs).
* It tracks input/output token counts, associated costs, and related metadata
* such as the user, course, exercise, and timestamps.
*/
public class LLMTokenUsage extends DomainObject {| public List<LLMTokenUsage> saveIrisTokenUsage(PyrisJob job, User user, Course course, List<PyrisLLMCostDTO> tokens) { | ||
| return saveIrisTokenUsage(job, null, null, user, course, tokens); | ||
| } | ||
|
|
||
| /** | ||
| * Overloaded method to save token usage without exercise. | ||
| * | ||
| * @param job used to create a unique traceId to group multiple LLM calls | ||
| * @param message IrisMessage to map the usage to an IrisMessage | ||
| * @param user to map the token cost to a user | ||
| * @param course to map the token to a course | ||
| * @param tokens token cost list of type PyrisLLMCostDTO | ||
| * @return list of the saved data | ||
| */ | ||
| public List<LLMTokenUsage> saveIrisTokenUsage(PyrisJob job, IrisMessage message, User user, Course course, List<PyrisLLMCostDTO> tokens) { | ||
| return saveIrisTokenUsage(job, message, null, user, course, tokens); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor overloaded methods to reduce code duplication
The overloaded saveIrisTokenUsage methods introduce code duplication by passing null for omitted parameters. This can be refactored to improve readability and maintainability. Consider using optional parameters or redesigning the method signatures to reduce duplication.
For example, you could create a single method that accepts a builder or use method chaining to handle optional parameters more elegantly.
| public List<LLMTokenUsage> saveIrisTokenUsage(PyrisJob job, IrisMessage message, Exercise exercise, User user, Course course, List<PyrisLLMCostDTO> tokens) { | ||
| List<LLMTokenUsage> tokenUsages = new ArrayList<>(); | ||
|
|
||
| for (PyrisLLMCostDTO cost : tokens) { |
There was a problem hiding this comment.
Add null check for tokens parameter to prevent NullPointerException
In the saveIrisTokenUsage method, the tokens parameter is not checked for null before use. If tokens is null, this will result in a NullPointerException when attempting to iterate over it. Consider adding a null check to handle this case gracefully.
Apply this diff to add a null check:
public List<LLMTokenUsage> saveIrisTokenUsage(PyrisJob job, IrisMessage message, Exercise exercise, User user, Course course, List<PyrisLLMCostDTO> tokens) {
+ if (tokens == null || tokens.isEmpty()) {
+ return new ArrayList<>();
+ }
List<LLMTokenUsage> tokenUsages = new ArrayList<>();📝 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.
| public List<LLMTokenUsage> saveIrisTokenUsage(PyrisJob job, IrisMessage message, Exercise exercise, User user, Course course, List<PyrisLLMCostDTO> tokens) { | |
| List<LLMTokenUsage> tokenUsages = new ArrayList<>(); | |
| for (PyrisLLMCostDTO cost : tokens) { | |
| public List<LLMTokenUsage> saveIrisTokenUsage(PyrisJob job, IrisMessage message, Exercise exercise, User user, Course course, List<PyrisLLMCostDTO> tokens) { | |
| if (tokens == null || tokens.isEmpty()) { | |
| return new ArrayList<>(); | |
| } | |
| List<LLMTokenUsage> tokenUsages = new ArrayList<>(); | |
| for (PyrisLLMCostDTO cost : tokens) { |
| /** | ||
| * Service for managing the LLMTokenUsage by all LLMs in Artemis | ||
| */ | ||
| @Service | ||
| @Profile(PROFILE_IRIS) | ||
| public class LLMTokenUsageService { | ||
|
|
||
| private final LLMTokenUsageRepository llmTokenUsageRepository; | ||
|
|
||
| public LLMTokenUsageService(LLMTokenUsageRepository llmTokenUsageRepository) { | ||
| this.llmTokenUsageRepository = llmTokenUsageRepository; | ||
| } | ||
|
|
||
| /** | ||
| * method saves the token usage to the database with a link to the IrisMessage | ||
| * messages of the same job are grouped together by saving the job id as a trace id | ||
| * | ||
| * @param job used to create a unique traceId to group multiple LLM calls | ||
| * @param message IrisMessage to map the usage to an IrisMessage | ||
| * @param exercise to map the token cost to an exercise | ||
| * @param user to map the token cost to a user | ||
| * @param course to map the token to a course | ||
| * @param tokens token cost list of type PyrisLLMCostDTO | ||
| * @return list of the saved data | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve JavaDoc comments for clarity and adherence to standards
The JavaDoc comments can be enhanced for better readability and to conform with JavaDoc standards:
- Start descriptions with a capital letter and end with a period.
- Use complete sentences for parameter descriptions.
- Ensure consistency and correctness in the documentation.
Apply this diff to improve the JavaDoc:
/**
- * method saves the token usage to the database with a link to the IrisMessage
- * messages of the same job are grouped together by saving the job id as a trace id
+ * Saves the token usage to the database with a link to the IrisMessage.
+ * Messages of the same job are grouped together by saving the job ID as a trace ID.
*
- * @param job used to create a unique traceId to group multiple LLM calls
- * @param message IrisMessage to map the usage to an IrisMessage
- * @param exercise to map the token cost to an exercise
- * @param user to map the token cost to a user
- * @param course to map the token to a course
- * @param tokens token cost list of type PyrisLLMCostDTO
- * @return list of the saved data
+ * @param job Used to create a unique trace ID to group multiple LLM calls.
+ * @param message IrisMessage to map the usage to an IrisMessage.
+ * @param exercise To map the token cost to an Exercise.
+ * @param user To map the token cost to a User.
+ * @param course To map the token to a Course.
+ * @param tokens Token cost list of type PyrisLLMCostDTO.
+ * @return List of the saved data.
*/📝 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.
| /** | |
| * Service for managing the LLMTokenUsage by all LLMs in Artemis | |
| */ | |
| @Service | |
| @Profile(PROFILE_IRIS) | |
| public class LLMTokenUsageService { | |
| private final LLMTokenUsageRepository llmTokenUsageRepository; | |
| public LLMTokenUsageService(LLMTokenUsageRepository llmTokenUsageRepository) { | |
| this.llmTokenUsageRepository = llmTokenUsageRepository; | |
| } | |
| /** | |
| * method saves the token usage to the database with a link to the IrisMessage | |
| * messages of the same job are grouped together by saving the job id as a trace id | |
| * | |
| * @param job used to create a unique traceId to group multiple LLM calls | |
| * @param message IrisMessage to map the usage to an IrisMessage | |
| * @param exercise to map the token cost to an exercise | |
| * @param user to map the token cost to a user | |
| * @param course to map the token to a course | |
| * @param tokens token cost list of type PyrisLLMCostDTO | |
| * @return list of the saved data | |
| */ | |
| /** | |
| * Service for managing the LLMTokenUsage by all LLMs in Artemis | |
| */ | |
| @Service | |
| @Profile(PROFILE_IRIS) | |
| public class LLMTokenUsageService { | |
| private final LLMTokenUsageRepository llmTokenUsageRepository; | |
| public LLMTokenUsageService(LLMTokenUsageRepository llmTokenUsageRepository) { | |
| this.llmTokenUsageRepository = llmTokenUsageRepository; | |
| } | |
| /** | |
| * Saves the token usage to the database with a link to the IrisMessage. | |
| * Messages of the same job are grouped together by saving the job ID as a trace ID. | |
| * | |
| * @param job Used to create a unique trace ID to group multiple LLM calls. | |
| * @param message IrisMessage to map the usage to an IrisMessage. | |
| * @param exercise To map the token cost to an Exercise. | |
| * @param user To map the token cost to a User. | |
| * @param course To map the token to a Course. | |
| * @param tokens Token cost list of type PyrisLLMCostDTO. | |
| * @return List of the saved data. | |
| */ |
| public List<LLMTokenUsage> saveIrisTokenUsage(PyrisJob job, IrisMessage message, Exercise exercise, User user, Course course, List<PyrisLLMCostDTO> tokens) { | ||
| List<LLMTokenUsage> tokenUsages = new ArrayList<>(); | ||
|
|
||
| for (PyrisLLMCostDTO cost : tokens) { | ||
| LLMTokenUsage llmTokenUsage = new LLMTokenUsage(); | ||
| if (message != null) { | ||
| llmTokenUsage.setIrisMessageId(message.getId()); | ||
| llmTokenUsage.setTime(message.getSentAt()); | ||
| } | ||
| if (user != null) { | ||
| llmTokenUsage.setUserId(user.getId()); | ||
| } | ||
| if (job != null) { | ||
| llmTokenUsage.setTraceId(job.jobId()); | ||
| } | ||
| llmTokenUsage.setServiceType(cost.pipeline()); | ||
| if (exercise != null) { | ||
| llmTokenUsage.setExerciseId(exercise.getId()); | ||
| } | ||
| if (course != null) { | ||
| llmTokenUsage.setCourseId(course.getId()); | ||
| } | ||
| llmTokenUsage.setNumInputTokens(cost.numInputTokens()); | ||
| llmTokenUsage.setCostPerMillionInputTokens(cost.costPerInputToken()); | ||
| llmTokenUsage.setNumOutputTokens(cost.numOutputTokens()); | ||
| llmTokenUsage.setCostPerMillionOutputTokens(cost.costPerOutputToken()); | ||
| llmTokenUsage.setModel(cost.modelInfo()); | ||
| tokenUsages.add(llmTokenUsage); | ||
| } | ||
| llmTokenUsageRepository.saveAll(tokenUsages); | ||
| return tokenUsages; |
There was a problem hiding this comment.
Ensure method is fault-tolerant and handle exceptions appropriately
The saveIrisTokenUsage method does not handle potential exceptions that may occur during database operations (e.g., saveAll). Since logging token usage is not mission-critical, failures in this method should not prevent the main operation from proceeding. Consider wrapping the database operation in a try-catch block to catch exceptions, log them, and allow the calling process to continue.
Apply this diff to add exception handling:
public List<LLMTokenUsage> saveIrisTokenUsage(PyrisJob job, IrisMessage message, Exercise exercise, User user, Course course, List<PyrisLLMCostDTO> tokens) {
+ try {
List<LLMTokenUsage> tokenUsages = new ArrayList<>();
for (PyrisLLMCostDTO cost : tokens) {
LLMTokenUsage llmTokenUsage = new LLMTokenUsage();
// ... set properties
tokenUsages.add(llmTokenUsage);
}
llmTokenUsageRepository.saveAll(tokenUsages);
return tokenUsages;
+ } catch (Exception e) {
+ // Log the exception, e.g., using a logging framework
+ log.error("Failed to save LLM token usage", e);
+ // Optionally, report the exception to a monitoring service
+ return new ArrayList<>();
+ }
}Committable suggestion was skipped due to low confidence.
| CompetencyExtractionJob job = new CompetencyExtractionJob(jobId, course.getId(), userUtilService.getUserByLogin(userLogin)); | ||
| irisCompetencyGenerationService.handleStatusUpdate(job, new PyrisCompetencyStatusUpdateDTO(stages, recommendations, null)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for the new null parameter in PyrisCompetencyStatusUpdateDTO.
With the addition of the new parameter (passed as null in this test case) to PyrisCompetencyStatusUpdateDTO, consider adding test cases to verify how handleStatusUpdate behaves when this parameter is non-null. This will ensure that both scenarios are tested, and the method handles the new parameter correctly.
src/main/java/de/tum/cit/aet/artemis/core/service/LLMTokenUsageService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (5)
- src/main/java/de/tum/cit/aet/artemis/core/service/LLMTokenUsageService.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.java (3 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (4 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (4 hunks)
- src/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/main/java/de/tum/cit/aet/artemis/core/service/LLMTokenUsageService.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java (1)
Pattern
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
📓 Learnings (1)
src/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java (1)
Learnt from: alexjoham PR: ls1intum/Artemis#9455 File: src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java:401-401 Timestamp: 2024-10-15T11:33:17.915Z Learning: In the Artemis project, when new fields are added to classes like `PyrisChatStatusUpdateDTO`, corresponding tests may be implemented in separate integration test classes such as `IrisChatTokenTrackingIntegrationTest`.
🔇 Additional comments (12)
src/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.java (3)
38-42: Dependencies are correctly injected via constructor.The addition of
LLMTokenUsageServiceandCourseRepositoryto the constructor aligns with dependency injection best practices and enhances the modularity of the service.
62-62: Previous suggestion still applies: Use a builder or factory method for optional parameters.Passing
nullvalues innew PyrisCompetencyStatusUpdateDTO(stages, null, null)may reduce code clarity. Consider using a builder or factory method to handle optional parameters more gracefully.
74-76: Previous suggestion still applies: Move course retrieval inside the conditional block.Since the
courseobject is only used whenstatusUpdate.tokens()is not null, moving the database retrieval inside the conditional block can improve performance by avoiding unnecessary queries.src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (2)
68-75: Constructor parameters are well-organizedThe addition of
llmTokenUsageServiceto the constructor parameters follows the existing pattern and maintains consistency. The parameter is correctly named in camelCase, adhering to Java naming conventions.
185-187: Token usage saving logic is refactored effectivelyMoving the
llmTokenUsageService.saveIrisTokenUsagecall outside the conditional blocks eliminates duplication and improves code clarity and maintainability.src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (4)
22-22: ImportingLLMTokenUsageServiceThe import statement for
LLMTokenUsageServiceis correctly added, ensuring that the class can use the token usage tracking functionalities.
47-48: Proper Declaration ofllmTokenUsageServiceThe
private final LLMTokenUsageService llmTokenUsageService;field is appropriately declared, adhering to the principle of immutability and promoting thread safety.
63-69: Constructor Updated with Dependency InjectionThe constructor has been updated to include
LLMTokenUsageService llmTokenUsageServiceas a parameter, and the assignmentthis.llmTokenUsageService = llmTokenUsageService;correctly initializes the field.
146-152: Effective Use of Builder Pattern insaveIrisTokenUsageThe
handleStatusUpdatemethod now uses a builder pattern when callingllmTokenUsageService.saveIrisTokenUsage, which enhances code readability and avoids passingnullvalues. This implementation aligns with best practices and addresses concerns about potentialNullPointerExceptions.src/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java (3)
241-244: Code Looks GoodThe method
createDefaultMockMessage()correctly creates a mock message with fixed content, adhering to the guideline (fixed_data: true).
246-249: Code Looks GoodThe method
createMockTextContent()uses fixed data, complying with the coding guideline to use fixed data in tests.
48-48: 🧹 Nitpick (assertive)Add JavaDoc Comment to Test Class
Adding a class-level JavaDoc comment to
IrisChatTokenTrackingIntegrationTestwill improve code readability and maintainability by providing a summary of the test class's purpose.Consider adding:
/** * Integration tests for tracking LLM token usage in Iris chat sessions. */⛔ Skipped due to learnings
Learnt from: alexjoham PR: ls1intum/Artemis#9455 File: src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java:401-401 Timestamp: 2024-10-15T11:33:17.915Z Learning: In the Artemis project, when new fields are added to classes like `PyrisChatStatusUpdateDTO`, corresponding tests may be implemented in separate integration test classes such as `IrisChatTokenTrackingIntegrationTest`.
| llmTokenUsage.setCourseId(course.getId()); | ||
| }); | ||
|
|
||
| llmTokenUsage.setTraceId(builder.getJob().jobId()); |
There was a problem hiding this comment.
Add null check for 'job' before using it
In the saveIrisTokenUsage method, builder.getJob() may be null, which will lead to a NullPointerException when calling builder.getJob().jobId(). Ensure that job is not null before using it, or handle the case when it is null appropriately.
Apply this diff to add the null check:
+ if (builder.getJob() == null) {
+ throw new IllegalArgumentException("Job must not be null");
+ }
llmTokenUsage.setTraceId(builder.getJob().jobId());📝 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.
| llmTokenUsage.setTraceId(builder.getJob().jobId()); | |
| if (builder.getJob() == null) { | |
| throw new IllegalArgumentException("Job must not be null"); | |
| } | |
| llmTokenUsage.setTraceId(builder.getJob().jobId()); |
| @BeforeEach | ||
| void initTestCase() throws GitAPIException, IOException, URISyntaxException { | ||
| userUtilService.addUsers(TEST_PREFIX, 2, 0, 0, 0); | ||
|
|
||
| course = programmingExerciseUtilService.addCourseWithOneProgrammingExercise(); | ||
| exercise = exerciseUtilService.getFirstExerciseWithType(course, ProgrammingExercise.class); | ||
| String projectKey = exercise.getProjectKey(); | ||
| exercise.setProjectType(ProjectType.PLAIN_GRADLE); | ||
| exercise.setTestRepositoryUri(localVCBaseUrl + "/git/" + projectKey + "/" + projectKey.toLowerCase() + "-tests.git"); | ||
| programmingExerciseBuildConfigRepository.save(exercise.getBuildConfig()); | ||
| programmingExerciseRepository.save(exercise); | ||
| exercise = programmingExerciseRepository.findWithAllParticipationsAndBuildConfigById(exercise.getId()).orElseThrow(); | ||
|
|
||
| // Set the correct repository URIs for the template and the solution participation. | ||
| String templateRepositorySlug = projectKey.toLowerCase() + "-exercise"; | ||
| TemplateProgrammingExerciseParticipation templateParticipation = exercise.getTemplateParticipation(); | ||
| templateParticipation.setRepositoryUri(localVCBaseUrl + "/git/" + projectKey + "/" + templateRepositorySlug + ".git"); | ||
| templateProgrammingExerciseParticipationRepository.save(templateParticipation); | ||
| String solutionRepositorySlug = projectKey.toLowerCase() + "-solution"; | ||
| SolutionProgrammingExerciseParticipation solutionParticipation = exercise.getSolutionParticipation(); | ||
| solutionParticipation.setRepositoryUri(localVCBaseUrl + "/git/" + projectKey + "/" + solutionRepositorySlug + ".git"); | ||
| solutionProgrammingExerciseParticipationRepository.save(solutionParticipation); | ||
|
|
||
| String assignmentRepositorySlug = projectKey.toLowerCase() + "-" + TEST_PREFIX + "student1"; | ||
|
|
||
| // Add a participation for student1. | ||
| ProgrammingExerciseStudentParticipation studentParticipation = participationUtilService.addStudentParticipationForProgrammingExercise(exercise, TEST_PREFIX + "student1"); | ||
| studentParticipation.setRepositoryUri(String.format(localVCBaseUrl + "/git/%s/%s.git", projectKey, assignmentRepositorySlug)); | ||
| studentParticipation.setBranch(defaultBranch); | ||
| programmingExerciseStudentParticipationRepository.save(studentParticipation); | ||
|
|
||
| // Prepare the repositories. | ||
| localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, templateRepositorySlug); | ||
| localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, projectKey.toLowerCase() + "-tests"); | ||
| localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, solutionRepositorySlug); | ||
| localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, assignmentRepositorySlug); | ||
|
|
||
| // Check that the repository folders were created in the file system for all base repositories. | ||
| localVCLocalCITestService.verifyRepositoryFoldersExist(exercise, localVCBasePath); | ||
|
|
||
| activateIrisGlobally(); | ||
| activateIrisFor(course); | ||
| activateIrisFor(exercise); | ||
| // Clean up the database | ||
| irisLLMTokenUsageRepository.deleteAll(); | ||
| pipelineDone = new AtomicBoolean(false); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider Minimizing Test Context Restarts
The initTestCase() method annotated with @BeforeEach performs extensive setup, including creating users, configuring exercises, and setting up repositories. According to the coding guideline (context_restart_minimize: true), minimizing context restarts improves test performance.
Consider refactoring static or unchanging setup into a @BeforeAll method:
@BeforeAll
static void initAll() throws Exception {
// Static setup code here
}Note: Since @BeforeAll methods need to be static, you may need to annotate the class with @TestInstance(Lifecycle.PER_CLASS) to use non-static methods.
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") | ||
| void testTokenTrackingHandledExerciseChat() throws Exception { | ||
| var irisSession = irisExerciseChatSessionService.createChatSessionForProgrammingExercise(exercise, userUtilService.getUserByLogin(TEST_PREFIX + "student1")); | ||
| var messageToSend = createDefaultMockMessage(irisSession); | ||
|
|
||
| var tokens = getMockLLMCosts(); | ||
|
|
||
| List<PyrisStageDTO> doneStage = new ArrayList<>(); | ||
| doneStage.add(new PyrisStageDTO("DoneTest", 10, PyrisStageState.DONE, "Done")); | ||
|
|
||
| irisRequestMockProvider.mockProgrammingExerciseChatResponse(dto -> { | ||
| assertThat(dto.settings().authenticationToken()).isNotNull(); | ||
|
|
||
| assertThatNoException().isThrownBy(() -> sendStatus(dto.settings().authenticationToken(), "Hello World", doneStage, null, tokens)); | ||
|
|
||
| pipelineDone.set(true); | ||
| }); | ||
|
|
||
| request.postWithoutResponseBody("/api/iris/sessions/" + irisSession.getId() + "/messages", messageToSend, HttpStatus.CREATED); | ||
|
|
||
| await().until(pipelineDone::get); | ||
|
|
||
| List<LLMTokenUsage> savedTokenUsages = irisLLMTokenUsageRepository.findAll(); | ||
| assertThat(savedTokenUsages).hasSize(5); | ||
| for (int i = 0; i < savedTokenUsages.size(); i++) { | ||
| LLMTokenUsage usage = savedTokenUsages.get(i); | ||
| PyrisLLMCostDTO expectedCost = tokens.get(i); | ||
|
|
||
| assertThat(usage.getModel()).isEqualTo(expectedCost.modelInfo()); | ||
| assertThat(usage.getNumInputTokens()).isEqualTo(expectedCost.numInputTokens()); | ||
| assertThat(usage.getNumOutputTokens()).isEqualTo(expectedCost.numOutputTokens()); | ||
| assertThat(usage.getCostPerMillionInputTokens()).isEqualTo(expectedCost.costPerInputToken()); | ||
| assertThat(usage.getCostPerMillionOutputTokens()).isEqualTo(expectedCost.costPerOutputToken()); | ||
| assertThat(usage.getServiceType()).isEqualTo(expectedCost.pipeline()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Common Test Logic to Reduce Duplication
The method testTokenTrackingHandledExerciseChat() contains logic that is similar to other test methods. To adhere to the DRY (Don't Repeat Yourself) principle and the coding guideline (test_size: small_specific), consider refactoring common code into a helper method.
Extract common logic into a helper method:
private void executeTokenTrackingTest(IrisSession irisSession, IrisMessage messageToSend, List<PyrisStageDTO> stages, String result) throws Exception {
irisRequestMockProvider.mockProgrammingExerciseChatResponse(dto -> {
assertThat(dto.settings().authenticationToken()).isNotNull();
assertThatNoException().isThrownBy(() -> sendStatus(dto.settings().authenticationToken(), result, stages, null, tokens));
pipelineDone.set(true);
});
request.postWithoutResponseBody("/api/iris/sessions/" + irisSession.getId() + "/messages", messageToSend, HttpStatus.CREATED);
await().until(pipelineDone::get);
List<LLMTokenUsage> savedTokenUsages = irisLLMTokenUsageRepository.findAll();
assertThat(savedTokenUsages).hasSize(5);
for (int i = 0; i < savedTokenUsages.size(); i++) {
LLMTokenUsage usage = savedTokenUsages.get(i);
PyrisLLMCostDTO expectedCost = tokens.get(i);
assertThat(usage.getModel()).isEqualTo(expectedCost.modelInfo());
assertThat(usage.getNumInputTokens()).isEqualTo(expectedCost.numInputTokens());
assertThat(usage.getNumOutputTokens()).isEqualTo(expectedCost.numOutputTokens());
assertThat(usage.getCostPerMillionInputTokens()).isEqualTo(expectedCost.costPerInputToken());
assertThat(usage.getCostPerMillionOutputTokens()).isEqualTo(expectedCost.costPerOutputToken());
assertThat(usage.getServiceType()).isEqualTo(expectedCost.pipeline());
}
}Then, update your test method:
void testTokenTrackingHandledExerciseChat() throws Exception {
var irisSession = irisExerciseChatSessionService.createChatSessionForProgrammingExercise(exercise, userUtilService.getUserByLogin(TEST_PREFIX + "student1"));
var messageToSend = createDefaultMockMessage(irisSession);
var tokens = getMockLLMCosts();
List<PyrisStageDTO> doneStage = List.of(new PyrisStageDTO("DoneTest", 10, PyrisStageState.DONE, "Done"));
- // Existing test logic...
+ executeTokenTrackingTest(irisSession, messageToSend, doneStage, "Hello World");
}| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") | ||
| void testTokenTrackingSavedExerciseChat() { | ||
|
|
||
| var irisSession = irisExerciseChatSessionService.createChatSessionForProgrammingExercise(exercise, userUtilService.getUserByLogin(TEST_PREFIX + "student1")); | ||
| var irisMessage = createDefaultMockMessage(irisSession); | ||
| irisMessageRepository.save(irisMessage); | ||
| String jobToken = pyrisJobService.addExerciseChatJob(course.getId(), exercise.getId(), irisSession.getId()); | ||
| PyrisJob job = pyrisJobService.getJob(jobToken); | ||
|
|
||
| var tokens = getMockLLMCosts(); | ||
|
|
||
| // Capture the saved token usages | ||
| List<LLMTokenUsage> returnedTokenUsages = llmTokenUsageService.saveIrisTokenUsage( | ||
| builder -> builder.withJob(job).withMessage(irisMessage).withExercise(exercise).withUser(irisSession.getUser()).withCourse(course).withTokens(tokens)); | ||
| assertThat(returnedTokenUsages).hasSize(5); | ||
| for (int i = 0; i < returnedTokenUsages.size(); i++) { | ||
| LLMTokenUsage usage = returnedTokenUsages.get(i); | ||
| PyrisLLMCostDTO expectedCost = tokens.get(i); | ||
|
|
||
| assertThat(usage.getModel()).isEqualTo(expectedCost.modelInfo()); | ||
| assertThat(usage.getNumInputTokens()).isEqualTo(expectedCost.numInputTokens()); | ||
| assertThat(usage.getNumOutputTokens()).isEqualTo(expectedCost.numOutputTokens()); | ||
| assertThat(usage.getCostPerMillionInputTokens()).isEqualTo(expectedCost.costPerInputToken()); | ||
| assertThat(usage.getCostPerMillionOutputTokens()).isEqualTo(expectedCost.costPerOutputToken()); | ||
| assertThat(usage.getServiceType()).isEqualTo(expectedCost.pipeline()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Common Test Logic to Reduce Duplication
Similar to the previous test method, testTokenTrackingSavedExerciseChat() shares common logic that can be refactored into a helper method to improve maintainability.
Consider refactoring the shared assertions and setup.
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") | ||
| void testTokenTrackingExerciseChatWithPipelineFail() throws Exception { | ||
| var irisSession = irisExerciseChatSessionService.createChatSessionForProgrammingExercise(exercise, userUtilService.getUserByLogin(TEST_PREFIX + "student1")); | ||
| var messageToSend = createDefaultMockMessage(irisSession); | ||
|
|
||
| var tokens = getMockLLMCosts(); | ||
|
|
||
| List<PyrisStageDTO> failedStages = new ArrayList<>(); | ||
| failedStages.add(new PyrisStageDTO("TestTokenFail", 10, PyrisStageState.ERROR, "Failed running pipeline")); | ||
|
|
||
| irisRequestMockProvider.mockProgrammingExerciseChatResponse(dto -> { | ||
| assertThat(dto.settings().authenticationToken()).isNotNull(); | ||
|
|
||
| assertThatNoException().isThrownBy(() -> sendStatus(dto.settings().authenticationToken(), null, failedStages, null, tokens)); | ||
|
|
||
| pipelineDone.set(true); | ||
| }); | ||
|
|
||
| request.postWithoutResponseBody("/api/iris/sessions/" + irisSession.getId() + "/messages", messageToSend, HttpStatus.CREATED); | ||
|
|
||
| await().until(pipelineDone::get); | ||
|
|
||
| List<LLMTokenUsage> savedTokenUsages = irisLLMTokenUsageRepository.findAll(); | ||
| assertThat(savedTokenUsages).hasSize(5); | ||
| for (int i = 0; i < savedTokenUsages.size(); i++) { | ||
| LLMTokenUsage usage = savedTokenUsages.get(i); | ||
| PyrisLLMCostDTO expectedCost = tokens.get(i); | ||
|
|
||
| assertThat(usage.getModel()).isEqualTo(expectedCost.modelInfo()); | ||
| assertThat(usage.getNumInputTokens()).isEqualTo(expectedCost.numInputTokens()); | ||
| assertThat(usage.getNumOutputTokens()).isEqualTo(expectedCost.numOutputTokens()); | ||
| assertThat(usage.getCostPerMillionInputTokens()).isEqualTo(expectedCost.costPerInputToken()); | ||
| assertThat(usage.getCostPerMillionOutputTokens()).isEqualTo(expectedCost.costPerOutputToken()); | ||
| assertThat(usage.getServiceType()).isEqualTo(expectedCost.pipeline()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Common Test Logic to Reduce Duplication
The method testTokenTrackingExerciseChatWithPipelineFail() also contains duplicated code. Refactoring common code into a helper method will adhere to best practices.
Use the previously suggested executeTokenTrackingTest method and adjust parameters as needed.
| private List<PyrisLLMCostDTO> getMockLLMCosts() { | ||
| List<PyrisLLMCostDTO> costs = new ArrayList<>(); | ||
| for (int i = 0; i < 5; i++) { | ||
| costs.add(new PyrisLLMCostDTO("test-llm", i * 10 + 5, i * 0.5f, i * 3 + 5, i * 0.12f, LLMServiceType.IRIS_CHAT_EXERCISE_MESSAGE.toString())); | ||
| } | ||
| return costs; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify getMockLLMCosts() Method Using Streams
The getMockLLMCosts() method can be simplified using Java Streams for better readability and conciseness.
Apply this change:
private List<PyrisLLMCostDTO> getMockLLMCosts() {
- List<PyrisLLMCostDTO> costs = new ArrayList<>();
- for (int i = 0; i < 5; i++) {
- costs.add(new PyrisLLMCostDTO("test-llm", i * 10 + 5, i * 0.5f, i * 3 + 5, i * 0.12f, LLMServiceType.IRIS_CHAT_EXERCISE_MESSAGE.toString()));
- }
- return costs;
+ return IntStream.range(0, 5)
+ .mapToObj(i -> new PyrisLLMCostDTO("test-llm", i * 10 + 5, i * 0.5f, i * 3 + 5, i * 0.12f, LLMServiceType.IRIS_CHAT_EXERCISE_MESSAGE.toString()))
+ .collect(Collectors.toList());
}Don't forget to import:
import java.util.stream.Collectors;
import java.util.stream.IntStream;| private void sendStatus(String jobId, String result, List<PyrisStageDTO> stages, List<String> suggestions, List<PyrisLLMCostDTO> tokens) throws Exception { | ||
| var headers = new HttpHeaders(new LinkedMultiValueMap<>(Map.of("Authorization", List.of("Bearer " + jobId)))); | ||
| request.postWithoutResponseBody("/api/public/pyris/pipelines/tutor-chat/runs/" + jobId + "/status", new PyrisChatStatusUpdateDTO(result, stages, suggestions, tokens), | ||
| HttpStatus.OK, headers); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine Exception Handling in sendStatus Method
The sendStatus method declares throws Exception, which is broad. According to best practices, it's advisable to handle or specify more specific exceptions.
Consider specifying the exact exceptions thrown or handling them within the method.
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsage.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 24
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (2)
src/main/resources/config/liquibase/changelog/20241018053210_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (14)
- src/main/java/de/tum/cit/aet/artemis/core/domain/LLMRequest.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/core/domain/LLMServiceType.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsageRequest.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsageTrace.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/core/repository/LLMTokenUsageTraceRepository.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/core/service/LLMTokenUsageService.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/dto/IrisChatWebsocketDTO.java (3 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.java (3 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatStatusUpdateDTO.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyStatusUpdateDTO.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (4 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (4 hunks)
- src/main/java/de/tum/cit/aet/artemis/iris/service/websocket/IrisChatWebsocketService.java (4 hunks)
- src/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java (1 hunks)
🧰 Additional context used
📓 Path-based instructions (14)
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMRequest.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/core/domain/LLMServiceType.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsageRequest.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsageTrace.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/core/repository/LLMTokenUsageTraceRepository.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/core/service/LLMTokenUsageService.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/iris/dto/IrisChatWebsocketDTO.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/IrisCompetencyGenerationService.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatStatusUpdateDTO.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyStatusUpdateDTO.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/iris/service/websocket/IrisChatWebsocketService.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java (1)
Pattern
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
📓 Learnings (1)
src/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java (1)
Learnt from: alexjoham PR: ls1intum/Artemis#9455 File: src/test/java/de/tum/cit/aet/artemis/iris/IrisTextExerciseChatMessageIntegrationTest.java:401-401 Timestamp: 2024-10-15T11:33:17.915Z Learning: In the Artemis project, when new fields are added to classes like `PyrisChatStatusUpdateDTO`, corresponding tests may be implemented in separate integration test classes such as `IrisChatTokenTrackingIntegrationTest`.
🔇 Additional comments (24)
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMServiceType.java (1)
1-1: LGTM: Package declaration is correct.The package declaration follows the correct naming convention for Java packages and adheres to the coding guidelines.
src/main/java/de/tum/cit/aet/artemis/core/repository/LLMTokenUsageTraceRepository.java (3)
1-1: LGTM: Package declaration is correct.The package declaration follows the expected structure and naming conventions for the project.
3-6: LGTM: Imports are correctly specified.The import statements are specific and avoid wildcard imports, adhering to the guideline of avoiding star imports. They import only the necessary classes for the interface.
8-10: LGTM: Interface declaration is correct. Consider custom query methods if needed.The interface declaration adheres to the project's coding guidelines:
- Follows CamelCase naming convention
- Has a single responsibility (managing LLMTokenUsageTrace entities)
- Uses @repository annotation for dependency injection
- Extends ArtemisJpaRepository for code reuse
The interface is currently empty, which is fine if no custom query methods are needed. However, consider adding custom query methods if there are specific data access patterns required for LLMTokenUsageTrace entities.
To ensure we're not missing any required custom query methods, let's check for usages of this repository:
✅ Verification successful
LGTM: No custom query methods required.
The usages of
LLMTokenUsageTraceRepositoryare limited to standard CRUD operations provided byArtemisJpaRepository. There is no indication that custom query methods are necessary at this time.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of LLMTokenUsageTraceRepository rg --type java "LLMTokenUsageTraceRepository" -A 5Length of output: 1917
src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/chat/PyrisChatStatusUpdateDTO.java (2)
7-7: LGTM: Import statement added correctly.The new import for
LLMRequestis necessary for the updated record definition and follows the coding guidelines.
Line range hint
1-13: Summary: Changes are appropriate and well-implemented.The modifications to
PyrisChatStatusUpdateDTO.javaalign well with the PR objectives. The newtokensfield has been added correctly, and the necessary import has been included. The changes follow Java best practices and the provided coding guidelines. The use of a record for this DTO is appropriate, promoting immutability and reducing boilerplate code.src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/dto/competency/PyrisCompetencyStatusUpdateDTO.java (2)
7-7: LGTM: Import statement added correctlyThe new import for
LLMRequestis correctly added and follows the coding guideline of avoiding star imports.
20-20: LGTM: Record definition updated correctlyThe
PyrisCompetencyStatusUpdateDTOrecord has been correctly updated to include the newtokensparameter. This change adheres to the coding guidelines by:
- Using CamelCase naming
- Following the single responsibility principle
- Using a Java record for a DTO
- Minimizing data in the DTO
src/main/java/de/tum/cit/aet/artemis/iris/dto/IrisChatWebsocketDTO.java (3)
10-10: LGTM: Import statement added correctly.The new import for
LLMRequestis necessary and follows the coding guidelines by avoiding star imports.
35-37: Update constructor Javadoc.The constructor has been correctly updated to include the new
tokensparameter. However, as mentioned in a previous review comment, the Javadoc for this constructor needs to be updated to include a description of thetokensparameter.Please update the constructor's Javadoc as follows:
/** * Creates a new IrisWebsocketDTO instance with the given parameters * Takes care of setting the type correctly * * @param message the IrisMessage (optional) * @param rateLimitInfo the rate limit information * @param stages the stages of the Pyris pipeline * @param suggestions the list of suggestions * @param tokens the list of LLM token usage information */
Line range hint
1-73: Overall implementation aligns with PR objectives.The changes to
IrisChatWebsocketDTOeffectively incorporate token usage tracking, which aligns with the PR objectives. The newtokensparameter has been added consistently to the record declaration and constructor. The implementation adheres to coding guidelines and best practices.Minor documentation updates are needed for both the record and constructor Javadocs to fully reflect these changes.
src/main/java/de/tum/cit/aet/artemis/iris/service/websocket/IrisChatWebsocketService.java (2)
10-10: LGTM: Import statement added correctlyThe new import for
LLMRequestis necessary and follows the coding guideline of avoiding star imports.
Line range hint
1-75: Overall assessment: Changes implement token usage tracking successfullyThe modifications to
IrisChatWebsocketServicesuccessfully implement the new feature of tracking token usage in Iris requests. The changes adhere to coding guidelines and maintain the class's single responsibility. The new parameterList<LLMRequest> tokensis consistently integrated into the relevant methods and constructor calls.Some minor suggestions have been made to improve code clarity and reduce the use of null values. Consider implementing these suggestions to further enhance the code quality.
The changes align well with the PR objectives of tracking token usage for Iris requests and lay a good foundation for future graphical interface implementations related to this feature.
src/main/java/de/tum/cit/aet/artemis/core/domain/LLMTokenUsageTrace.java (1)
74-80:⚠️ Potential issuePrevent Potential NullPointerException in
getUserId()The getter method
public long getUserId()returns a primitivelong, but the corresponding fielduserIdis an object of typeLong. IfuserIdisnull, this will cause aNullPointerException. Ensure thatuserIdis always initialized and cannot benull, or adjust the return type toLongand handle possiblenullvalues appropriately.If changing the field to a primitive type as suggested, the getter method will be consistent. Otherwise, modify the getter to handle
nullvalues:- public long getUserId() { + public Long getUserId() { return userId; }Likely invalid or redundant comment.
src/main/java/de/tum/cit/aet/artemis/core/service/LLMTokenUsageService.java (1)
1-119: Overall code quality is commendableThe
LLMTokenUsageServiceis well-structured, and the use of the builder pattern enhances readability and extensibility.src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisExerciseChatSessionService.java (4)
49-50: Dependency Injection Implemented CorrectlyThe
llmTokenUsageServiceis added as aprivate finalmember and is injected via the constructor, adhering to the dependency injection pattern and best practices outlined in the coding guidelines.
69-76: Constructor Parameters Correctly UpdatedThe constructor has been updated to include the
LLMTokenUsageServiceparameter. This ensures that the service is properly injected and available for use within the class.
174-194: Token Usage Tracking Integrated SuccessfullyThe implementation of token usage tracking within the
handleStatusUpdatemethod correctly handles different scenarios based on the availability ofresultandtokens. The usage ofllmTokenUsageService.saveLLMTokenUsageappropriately saves token usage data with all necessary information.
174-174:⚠️ Potential issueVerify Lazy Loading of
exerciseandcourseAssociationsThe
sessionretrieved byirisSessionRepository.findByIdWithMessagesAndContents(job.sessionId())might not eagerly load theexerciseand associatedcourse. This could lead toLazyInitializationExceptionorNullPointerExceptionwhen accessingsession.getExercise()and subsequent calls.To confirm that the
exerciseandcourseare properly loaded with the session, please run the following script:Expected Result:
- The
findByIdWithMessagesAndContentsmethod definition should include an entity graph or join fetch forexerciseandcourse.- If not, consider modifying the repository method to eagerly fetch these associations.
src/test/java/de/tum/cit/aet/artemis/iris/IrisChatTokenTrackingIntegrationTest.java (1)
154-157: Mock Text Content Correctly Uses Fixed DataThe
createMockTextContent()method provides fixed, deterministic text data, which adheres to the coding guidelinefixed_data: true. This ensures that tests are predictable and reproducible.src/main/java/de/tum/cit/aet/artemis/iris/service/session/IrisCourseChatSessionService.java (4)
48-48: Field Added for Token Usage TrackingThe addition of the
llmTokenUsageServicefield is appropriate for integrating token usage tracking into the service.
64-70: Constructor Updated with New DependencyThe constructor now includes the
LLMTokenUsageService llmTokenUsageServiceparameter and initializes the corresponding field. This ensures that the service has access to the token usage functionality.
147-148: Token Usage Saved When Result Is PresentIn the
handleStatusUpdatemethod, token usage is correctly saved when a result is available, associating it with theIrisMessage. This ensures accurate tracking of token consumption linked to specific messages.
152-153: Token Usage Saved When Result Is AbsentEven when there's no result in
handleStatusUpdate, the token usage is appropriately saved without an associatedIrisMessage. This allows for comprehensive tracking of all token usage scenarios.
| /** | ||
| * Enum representing different types of LLM (Large Language Model) services used in the system. | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance JavaDoc with details about enum constants.
The JavaDoc comment provides a good overview of the enum's purpose. However, consider adding more details about the enum constants to improve documentation.
Here's a suggested enhancement:
/**
* Enum representing different types of LLM (Large Language Model) services used in the system.
* <p>
* IRIS: Represents the IRIS LLM service.
* ATHENA: Represents the ATHENA LLM service.
*/| public enum LLMServiceType { | ||
| IRIS, ATHENA | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing constants and improve enum readability.
The enum declaration follows the correct naming conventions. However, there are two issues to address:
- Missing constants: Based on the PR objectives and previous discussions, several constants are missing (e.g., ATHENA_PRELIMINARY_FEEDBACK, IRIS_CODE_FEEDBACK, etc.).
- Readability: Place each constant on a separate line for improved readability.
Here's a suggested refactor:
public enum LLMServiceType {
ATHENA_PRELIMINARY_FEEDBACK,
ATHENA_FEEDBACK_SUGGESTION,
IRIS_CODE_FEEDBACK,
IRIS_CHAT_COURSE_MESSAGE,
IRIS_CHAT_EXERCISE_MESSAGE,
IRIS_INTERACTION_SUGGESTION,
IRIS_CHAT_LECTURE_MESSAGE,
IRIS_COMPETENCY_GENERATION,
IRIS_CITATION_PIPELINE,
IRIS_LECTURE_RETRIEVAL_PIPELINE,
IRIS_LECTURE_INGESTION,
NOT_SET
}Please ensure all necessary constants are included and consider adding brief comments for each constant if their purposes are not self-evident.
| package de.tum.cit.aet.artemis.core.domain; | ||
|
|
||
| /** | ||
| * Enum representing different types of LLM (Large Language Model) services used in the system. | ||
| */ | ||
| public enum LLMServiceType { | ||
| IRIS, ATHENA | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Summary: Enum structure needs revision and expansion.
Overall, the file follows most coding guidelines, but there are areas for improvement:
- Enhance the JavaDoc with details about each enum constant.
- Add the missing constants as per the PR objectives and previous discussions.
- Improve readability by placing each constant on a separate line.
These changes will make the enum more comprehensive and easier to maintain in the future.
| package de.tum.cit.aet.artemis.core.domain; | ||
|
|
||
| public record LLMRequest(String model, int numInputTokens, float costPerMillionInputToken, int numOutputTokens, float costPerMillionOutputToken, String pipelineId) { | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add JavaDoc comments for better documentation.
The LLMRequest record looks good and adheres to the coding guidelines. To improve code documentation and maintainability, consider adding JavaDoc comments to describe the purpose of the record and its fields.
Here's a suggested improvement:
package de.tum.cit.aet.artemis.core.domain;
+/**
+ * Represents a request to a Language Learning Model (LLM).
+ * This record encapsulates all necessary information about an LLM request,
+ * including token counts, associated costs, and identifiers.
+ */
public record LLMRequest(
+ /** The name or identifier of the LLM model being used. */
String model,
+ /** The number of input tokens in the request. */
int numInputTokens,
+ /** The cost per million input tokens. */
float costPerMillionInputToken,
+ /** The number of output tokens generated. */
int numOutputTokens,
+ /** The cost per million output tokens. */
float costPerMillionOutputToken,
+ /** The identifier of the pipeline processing this request. */
String pipelineId
) {}This addition will provide clear documentation for developers working with this class in the future.
📝 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.
| package de.tum.cit.aet.artemis.core.domain; | |
| public record LLMRequest(String model, int numInputTokens, float costPerMillionInputToken, int numOutputTokens, float costPerMillionOutputToken, String pipelineId) { | |
| } | |
| package de.tum.cit.aet.artemis.core.domain; | |
| /** | |
| * Represents a request to a Language Learning Model (LLM). | |
| * This record encapsulates all necessary information about an LLM request, | |
| * including token counts, associated costs, and identifiers. | |
| */ | |
| public record LLMRequest( | |
| /** The name or identifier of the LLM model being used. */ | |
| String model, | |
| /** The number of input tokens in the request. */ | |
| int numInputTokens, | |
| /** The cost per million input tokens. */ | |
| float costPerMillionInputToken, | |
| /** The number of output tokens generated. */ | |
| int numOutputTokens, | |
| /** The cost per million output tokens. */ | |
| float costPerMillionOutputToken, | |
| /** The identifier of the pipeline processing this request. */ | |
| String pipelineId | |
| ) {} |
| import de.tum.cit.aet.artemis.iris.service.pyris.dto.status.PyrisStageDTO; | ||
|
|
||
| @JsonInclude(JsonInclude.Include.NON_EMPTY) | ||
| public record PyrisChatStatusUpdateDTO(String result, List<PyrisStageDTO> stages, List<String> suggestions) { | ||
| public record PyrisChatStatusUpdateDTO(String result, List<PyrisStageDTO> stages, List<String> suggestions, List<LLMRequest> tokens) { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: Record definition updated correctly.
The PyrisChatStatusUpdateDTO record has been properly updated to include the new tokens field. This change aligns with the PR objectives and follows Java best practices.
Consider adding a JavaDoc comment to the record to describe its purpose and the new tokens field. This would improve code documentation and maintainability. For example:
/**
* Data Transfer Object for Pyris chat status updates.
*
* @param result The result of the chat operation.
* @param stages List of stages in the chat process.
* @param suggestions List of suggestions provided.
* @param tokens List of LLM requests containing token usage information.
*/
public record PyrisChatStatusUpdateDTO(String result, List<PyrisStageDTO> stages, List<String> suggestions, List<LLMRequest> tokens) {
}| /** | ||
| * method saves the token usage to the database | ||
| * | ||
| * @param llmRequests List of LLM requests | ||
| * @param serviceType type of the LLM service | ||
| * @param builderFunction of type Function<IrisTokenUsageBuilder, IrisTokenUsageBuilder> using IrisTokenUsageBuilder | ||
| * @return saved LLMTokenUsage as a List | ||
| */ |
There was a problem hiding this comment.
Correct JavaDoc comments for accuracy and clarity
The JavaDoc for the saveLLMTokenUsage method contains inaccuracies:
- The method description should start with a capital letter and be in third person singular.
- The parameter description incorrectly references
IrisTokenUsageBuilderinstead ofLLMTokenUsageBuilder. - The
@returntag mentions aList, but the method returns aLLMTokenUsageTrace.
Apply this diff to correct the JavaDoc:
/**
- * method saves the token usage to the database
+ * Saves the token usage to the database.
*
* @param llmRequests List of LLM requests
* @param serviceType type of the LLM service
- * @param builderFunction of type Function<IrisTokenUsageBuilder, IrisTokenUsageBuilder> using IrisTokenUsageBuilder
- * @return saved LLMTokenUsage as a List
+ * @param builderFunction Function to build an instance of LLMTokenUsageBuilder
+ * @return Saved LLMTokenUsageTrace instance
*/📝 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.
| /** | |
| * method saves the token usage to the database | |
| * | |
| * @param llmRequests List of LLM requests | |
| * @param serviceType type of the LLM service | |
| * @param builderFunction of type Function<IrisTokenUsageBuilder, IrisTokenUsageBuilder> using IrisTokenUsageBuilder | |
| * @return saved LLMTokenUsage as a List | |
| */ | |
| /** | |
| * Saves the token usage to the database. | |
| * | |
| * @param llmRequests List of LLM requests | |
| * @param serviceType type of the LLM service | |
| * @param builderFunction Function to build an instance of LLMTokenUsageBuilder | |
| * @return Saved LLMTokenUsageTrace instance | |
| */ |
| public LLMTokenUsageTrace saveLLMTokenUsage(List<LLMRequest> llmRequests, LLMServiceType serviceType, Function<LLMTokenUsageBuilder, LLMTokenUsageBuilder> builderFunction) { | ||
| LLMTokenUsageTrace llmTokenUsageTrace = new LLMTokenUsageTrace(); | ||
| llmTokenUsageTrace.setServiceType(serviceType); | ||
|
|
||
| LLMTokenUsageBuilder builder = builderFunction.apply(new LLMTokenUsageBuilder()); | ||
| builder.getIrisMessageID().ifPresent(llmTokenUsageTrace::setIrisMessageId); | ||
| builder.getUser().ifPresent(user -> { | ||
| llmTokenUsageTrace.setUserId(user.getId()); | ||
| }); | ||
| builder.getExercise().ifPresent(exercise -> { | ||
| llmTokenUsageTrace.setExerciseId(exercise.getId()); | ||
| }); | ||
| builder.getCourse().ifPresent(course -> { | ||
| llmTokenUsageTrace.setCourseId(course.getId()); | ||
| }); | ||
|
|
||
| Set<LLMTokenUsageRequest> llmRequestsSet = llmTokenUsageTrace.getLLMRequests(); | ||
| for (LLMRequest llmRequest : llmRequests) { | ||
| LLMTokenUsageRequest llmTokenUsageRequest = new LLMTokenUsageRequest(); | ||
| llmTokenUsageRequest.setModel(llmRequest.model()); | ||
| llmTokenUsageRequest.setNumInputTokens(llmRequest.numInputTokens()); | ||
| llmTokenUsageRequest.setNumOutputTokens(llmRequest.numOutputTokens()); | ||
| llmTokenUsageRequest.setCostPerMillionInputTokens(llmRequest.costPerMillionInputToken()); | ||
| llmTokenUsageRequest.setCostPerMillionOutputTokens(llmRequest.costPerMillionOutputToken()); | ||
| llmTokenUsageRequest.setServicePipelineId(llmRequest.pipelineId()); | ||
| llmTokenUsageRequest.setTrace(llmTokenUsageTrace); | ||
| llmRequestsSet.add(llmTokenUsageRequest); | ||
| } | ||
| return llmTokenUsageTraceRepository.save(llmTokenUsageTrace); | ||
| } |
There was a problem hiding this comment.
Add null check for 'llmRequests' parameter
The llmRequests parameter is not null-checked. Passing a null value will cause a NullPointerException.
Apply this diff to ensure llmRequests is not null or empty:
public LLMTokenUsageTrace saveLLMTokenUsage(List<LLMRequest> llmRequests, LLMServiceType serviceType, Function<LLMTokenUsageBuilder, LLMTokenUsageBuilder> builderFunction) {
+ if (llmRequests == null || llmRequests.isEmpty()) {
+ throw new IllegalArgumentException("llmRequests must not be null or empty");
+ }
LLMTokenUsageTrace llmTokenUsageTrace = new LLMTokenUsageTrace();
llmTokenUsageTrace.setServiceType(serviceType);
// Rest of the code...📝 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.
| public LLMTokenUsageTrace saveLLMTokenUsage(List<LLMRequest> llmRequests, LLMServiceType serviceType, Function<LLMTokenUsageBuilder, LLMTokenUsageBuilder> builderFunction) { | |
| LLMTokenUsageTrace llmTokenUsageTrace = new LLMTokenUsageTrace(); | |
| llmTokenUsageTrace.setServiceType(serviceType); | |
| LLMTokenUsageBuilder builder = builderFunction.apply(new LLMTokenUsageBuilder()); | |
| builder.getIrisMessageID().ifPresent(llmTokenUsageTrace::setIrisMessageId); | |
| builder.getUser().ifPresent(user -> { | |
| llmTokenUsageTrace.setUserId(user.getId()); | |
| }); | |
| builder.getExercise().ifPresent(exercise -> { | |
| llmTokenUsageTrace.setExerciseId(exercise.getId()); | |
| }); | |
| builder.getCourse().ifPresent(course -> { | |
| llmTokenUsageTrace.setCourseId(course.getId()); | |
| }); | |
| Set<LLMTokenUsageRequest> llmRequestsSet = llmTokenUsageTrace.getLLMRequests(); | |
| for (LLMRequest llmRequest : llmRequests) { | |
| LLMTokenUsageRequest llmTokenUsageRequest = new LLMTokenUsageRequest(); | |
| llmTokenUsageRequest.setModel(llmRequest.model()); | |
| llmTokenUsageRequest.setNumInputTokens(llmRequest.numInputTokens()); | |
| llmTokenUsageRequest.setNumOutputTokens(llmRequest.numOutputTokens()); | |
| llmTokenUsageRequest.setCostPerMillionInputTokens(llmRequest.costPerMillionInputToken()); | |
| llmTokenUsageRequest.setCostPerMillionOutputTokens(llmRequest.costPerMillionOutputToken()); | |
| llmTokenUsageRequest.setServicePipelineId(llmRequest.pipelineId()); | |
| llmTokenUsageRequest.setTrace(llmTokenUsageTrace); | |
| llmRequestsSet.add(llmTokenUsageRequest); | |
| } | |
| return llmTokenUsageTraceRepository.save(llmTokenUsageTrace); | |
| } | |
| public LLMTokenUsageTrace saveLLMTokenUsage(List<LLMRequest> llmRequests, LLMServiceType serviceType, Function<LLMTokenUsageBuilder, LLMTokenUsageBuilder> builderFunction) { | |
| if (llmRequests == null || llmRequests.isEmpty()) { | |
| throw new IllegalArgumentException("llmRequests must not be null or empty"); | |
| } | |
| LLMTokenUsageTrace llmTokenUsageTrace = new LLMTokenUsageTrace(); | |
| llmTokenUsageTrace.setServiceType(serviceType); | |
| LLMTokenUsageBuilder builder = builderFunction.apply(new LLMTokenUsageBuilder()); | |
| builder.getIrisMessageID().ifPresent(llmTokenUsageTrace::setIrisMessageId); | |
| builder.getUser().ifPresent(user -> { | |
| llmTokenUsageTrace.setUserId(user.getId()); | |
| }); | |
| builder.getExercise().ifPresent(exercise -> { | |
| llmTokenUsageTrace.setExerciseId(exercise.getId()); | |
| }); | |
| builder.getCourse().ifPresent(course -> { | |
| llmTokenUsageTrace.setCourseId(course.getId()); | |
| }); | |
| Set<LLMTokenUsageRequest> llmRequestsSet = llmTokenUsageTrace.getLLMRequests(); | |
| for (LLMRequest llmRequest : llmRequests) { | |
| LLMTokenUsageRequest llmTokenUsageRequest = new LLMTokenUsageRequest(); | |
| llmTokenUsageRequest.setModel(llmRequest.model()); | |
| llmTokenUsageRequest.setNumInputTokens(llmRequest.numInputTokens()); | |
| llmTokenUsageRequest.setNumOutputTokens(llmRequest.numOutputTokens()); | |
| llmTokenUsageRequest.setCostPerMillionInputTokens(llmRequest.costPerMillionInputToken()); | |
| llmTokenUsageRequest.setCostPerMillionOutputTokens(llmRequest.costPerMillionOutputToken()); | |
| llmTokenUsageRequest.setServicePipelineId(llmRequest.pipelineId()); | |
| llmTokenUsageRequest.setTrace(llmTokenUsageTrace); | |
| llmRequestsSet.add(llmTokenUsageRequest); | |
| } | |
| return llmTokenUsageTraceRepository.save(llmTokenUsageTrace); | |
| } |
| * var irisSession = irisExerciseChatSessionService.createChatSessionForProgrammingExercise(exercise, userUtilService.getUserByLogin(TEST_PREFIX + "student1")); | ||
| * var messageToSend = createDefaultMockMessage(irisSession); | ||
| * var tokens = getMockLLMCosts(); | ||
| * List<PyrisStageDTO> doneStage = new ArrayList<>(); | ||
| * doneStage.add(new PyrisStageDTO("DoneTest", 10, PyrisStageState.DONE, "Done")); | ||
| * irisRequestMockProvider.mockProgrammingExerciseChatResponse(dto -> { | ||
| * assertThat(dto.settings().authenticationToken()).isNotNull(); | ||
| * assertThatNoException().isThrownBy(() -> sendStatus(dto.settings().authenticationToken(), "Hello World", doneStage, null, tokens)); | ||
| * pipelineDone.set(true); | ||
| * }); | ||
| * request.postWithoutResponseBody("/api/iris/sessions/" + irisSession.getId() + "/messages", messageToSend, HttpStatus.CREATED); | ||
| * await().until(pipelineDone::get); | ||
| * List<LLMTokenUsage> savedTokenUsages = irisLLMTokenUsageRepository.findAll(); | ||
| * assertThat(savedTokenUsages).hasSize(5); | ||
| * for (int i = 0; i < savedTokenUsages.size(); i++) { | ||
| * LLMTokenUsage usage = savedTokenUsages.get(i); | ||
| * PyrisLLMCostDTO expectedCost = tokens.get(i); | ||
| * assertThat(usage.getModel()).isEqualTo(expectedCost.modelInfo()); | ||
| * assertThat(usage.getNumInputTokens()).isEqualTo(expectedCost.numInputTokens()); | ||
| * assertThat(usage.getNumOutputTokens()).isEqualTo(expectedCost.numOutputTokens()); | ||
| * assertThat(usage.getCostPerMillionInputTokens()).isEqualTo(expectedCost.costPerInputToken()); | ||
| * assertThat(usage.getCostPerMillionOutputTokens()).isEqualTo(expectedCost.costPerOutputToken()); | ||
| * assertThat(usage.getServiceType()).isEqualTo(expectedCost.pipeline()); | ||
| * } | ||
| * } | ||
| * @Test | ||
| * @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") | ||
| * void testTokenTrackingSavedExerciseChat() { | ||
| * var irisSession = irisExerciseChatSessionService.createChatSessionForProgrammingExercise(exercise, userUtilService.getUserByLogin(TEST_PREFIX + "student1")); | ||
| * var irisMessage = createDefaultMockMessage(irisSession); | ||
| * irisMessageRepository.save(irisMessage); | ||
| * String jobToken = pyrisJobService.addExerciseChatJob(course.getId(), exercise.getId(), irisSession.getId()); | ||
| * PyrisJob job = pyrisJobService.getJob(jobToken); | ||
| * var tokens = getMockLLMCosts(); | ||
| * // Capture the saved token usages | ||
| * List<LLMTokenUsageTrace> returnedTokenUsages = llmTokenUsageService.saveLLMTokenUsage( | ||
| * builder -> builder.withMessage(irisMessage).withExercise(exercise).withUser(irisSession.getUser()).withCourse(course).withTokens(tokens)); | ||
| * assertThat(returnedTokenUsages).hasSize(5); | ||
| * for (int i = 0; i < returnedTokenUsages.size(); i++) { | ||
| * LLMTokenUsage usage = returnedTokenUsages.get(i); | ||
| * PyrisLLMCostDTO expectedCost = tokens.get(i); | ||
| * assertThat(usage.getModel()).isEqualTo(expectedCost.modelInfo()); | ||
| * assertThat(usage.getNumInputTokens()).isEqualTo(expectedCost.numInputTokens()); | ||
| * assertThat(usage.getNumOutputTokens()).isEqualTo(expectedCost.numOutputTokens()); | ||
| * assertThat(usage.getCostPerMillionInputTokens()).isEqualTo(expectedCost.costPerInputToken()); | ||
| * assertThat(usage.getCostPerMillionOutputTokens()).isEqualTo(expectedCost.costPerOutputToken()); | ||
| * assertThat(usage.getServiceType()).isEqualTo(expectedCost.pipeline()); | ||
| * } | ||
| * } | ||
| * @Test | ||
| * @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") | ||
| * void testTokenTrackingExerciseChatWithPipelineFail() throws Exception { | ||
| * var irisSession = irisExerciseChatSessionService.createChatSessionForProgrammingExercise(exercise, userUtilService.getUserByLogin(TEST_PREFIX + "student1")); | ||
| * var messageToSend = createDefaultMockMessage(irisSession); | ||
| * var tokens = getMockLLMCosts(); | ||
| * List<PyrisStageDTO> failedStages = new ArrayList<>(); | ||
| * failedStages.add(new PyrisStageDTO("TestTokenFail", 10, PyrisStageState.ERROR, "Failed running pipeline")); | ||
| * irisRequestMockProvider.mockProgrammingExerciseChatResponse(dto -> { | ||
| * assertThat(dto.settings().authenticationToken()).isNotNull(); | ||
| * assertThatNoException().isThrownBy(() -> sendStatus(dto.settings().authenticationToken(), null, failedStages, null, tokens)); | ||
| * pipelineDone.set(true); | ||
| * }); | ||
| * request.postWithoutResponseBody("/api/iris/sessions/" + irisSession.getId() + "/messages", messageToSend, HttpStatus.CREATED); | ||
| * await().until(pipelineDone::get); | ||
| * List<LLMTokenUsage> savedTokenUsages = irisLLMTokenUsageRepository.findAll(); | ||
| * assertThat(savedTokenUsages).hasSize(5); | ||
| * for (int i = 0; i < savedTokenUsages.size(); i++) { | ||
| * LLMTokenUsage usage = savedTokenUsages.get(i); | ||
| * PyrisLLMCostDTO expectedCost = tokens.get(i); | ||
| * assertThat(usage.getModel()).isEqualTo(expectedCost.modelInfo()); | ||
| * assertThat(usage.getNumInputTokens()).isEqualTo(expectedCost.numInputTokens()); | ||
| * assertThat(usage.getNumOutputTokens()).isEqualTo(expectedCost.numOutputTokens()); | ||
| * assertThat(usage.getCostPerMillionInputTokens()).isEqualTo(expectedCost.costPerInputToken()); | ||
| * assertThat(usage.getCostPerMillionOutputTokens()).isEqualTo(expectedCost.costPerOutputToken()); | ||
| * assertThat(usage.getServiceType()).isEqualTo(expectedCost.pipeline()); | ||
| * } | ||
| * } | ||
| * private List<LLMRequest> getMockLLMCosts() { | ||
| * List<LLMRequest> costs = new ArrayList<>(); | ||
| * for (int i = 0; i < 5; i++) { | ||
| * costs.add(new LLMRequest("test-llm", i * 10 + 5, i * 0.5f, i * 3 + 5, i * 0.12f, "IRIS_CHAT_EXERCISE_MESSAGE")); | ||
| * } | ||
| * return costs; | ||
| * } | ||
| * private IrisMessage createDefaultMockMessage(IrisSession irisSession) { | ||
| * var messageToSend = irisSession.newMessage(); | ||
| * messageToSend.addContent(createMockTextContent(), createMockTextContent(), createMockTextContent()); | ||
| * return messageToSend; | ||
| * } | ||
| * private IrisMessageContent createMockTextContent() { | ||
| * var text = "The happy dog jumped over the lazy dog."; | ||
| * return new IrisTextMessageContent(text); | ||
| * } | ||
| * private void sendStatus(String jobId, String result, List<PyrisStageDTO> stages, List<String> suggestions, List<PyrisLLMCostDTO> tokens) throws Exception { | ||
| * var headers = new HttpHeaders(new LinkedMultiValueMap<>(Map.of("Authorization", List.of("Bearer " + jobId)))); | ||
| * request.postWithoutResponseBody("/api/public/pyris/pipelines/tutor-chat/runs/" + jobId + "/status", new PyrisChatStatusUpdateDTO(result, stages, suggestions, tokens), | ||
| * HttpStatus.OK, headers); | ||
| * } | ||
| * } | ||
| */ |
There was a problem hiding this comment.
Uncomment the Test Class to Enable Execution
The entire IrisChatTokenTrackingIntegrationTest class is enclosed within block comments (/* ... */), which prevents the code from being compiled and executed. To run the tests and ensure the functionality is properly validated, please remove the comment delimiters.
Apply this diff to uncomment the class:
-/*
package de.tum.cit.aet.artemis.iris;
class IrisChatTokenTrackingIntegrationTest extends AbstractIrisIntegrationTest {
// ... rest of the code ...
}
-*/Committable suggestion was skipped due to low confidence.
| * List<LLMTokenUsageTrace> returnedTokenUsages = llmTokenUsageService.saveLLMTokenUsage( | ||
| * builder -> builder.withMessage(irisMessage).withExercise(exercise).withUser(irisSession.getUser()).withCourse(course).withTokens(tokens)); | ||
| * assertThat(returnedTokenUsages).hasSize(5); | ||
| * for (int i = 0; i < returnedTokenUsages.size(); i++) { | ||
| * LLMTokenUsage usage = returnedTokenUsages.get(i); | ||
| * PyrisLLMCostDTO expectedCost = tokens.get(i); | ||
| * assertThat(usage.getModel()).isEqualTo(expectedCost.modelInfo()); | ||
| * assertThat(usage.getNumInputTokens()).isEqualTo(expectedCost.numInputTokens()); | ||
| * assertThat(usage.getNumOutputTokens()).isEqualTo(expectedCost.numOutputTokens()); | ||
| * assertThat(usage.getCostPerMillionInputTokens()).isEqualTo(expectedCost.costPerInputToken()); | ||
| * assertThat(usage.getCostPerMillionOutputTokens()).isEqualTo(expectedCost.costPerOutputToken()); | ||
| * assertThat(usage.getServiceType()).isEqualTo(expectedCost.pipeline()); | ||
| * } |
There was a problem hiding this comment.
Type Mismatch Between LLMTokenUsageTrace and LLMTokenUsage
In the testTokenTrackingSavedExerciseChat() method, you declare returnedTokenUsages as List<LLMTokenUsageTrace>, but later iterate over it using a variable of type LLMTokenUsage. This can lead to a type mismatch error since LLMTokenUsageTrace may not be directly assignable to LLMTokenUsage.
Apply this diff to correct the type:
- List<LLMTokenUsageTrace> returnedTokenUsages = llmTokenUsageService.saveLLMTokenUsage(
+ List<LLMTokenUsage> returnedTokenUsages = llmTokenUsageService.saveLLMTokenUsage(
builder -> builder.withMessage(irisMessage).withExercise(exercise).withUser(irisSession.getUser()).withCourse(course).withTokens(tokens));Ensure that the saveLLMTokenUsage method returns a List<LLMTokenUsage> or adjust your code accordingly if the return type is intended to be LLMTokenUsageTrace.
📝 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.
| * List<LLMTokenUsageTrace> returnedTokenUsages = llmTokenUsageService.saveLLMTokenUsage( | |
| * builder -> builder.withMessage(irisMessage).withExercise(exercise).withUser(irisSession.getUser()).withCourse(course).withTokens(tokens)); | |
| * assertThat(returnedTokenUsages).hasSize(5); | |
| * for (int i = 0; i < returnedTokenUsages.size(); i++) { | |
| * LLMTokenUsage usage = returnedTokenUsages.get(i); | |
| * PyrisLLMCostDTO expectedCost = tokens.get(i); | |
| * assertThat(usage.getModel()).isEqualTo(expectedCost.modelInfo()); | |
| * assertThat(usage.getNumInputTokens()).isEqualTo(expectedCost.numInputTokens()); | |
| * assertThat(usage.getNumOutputTokens()).isEqualTo(expectedCost.numOutputTokens()); | |
| * assertThat(usage.getCostPerMillionInputTokens()).isEqualTo(expectedCost.costPerInputToken()); | |
| * assertThat(usage.getCostPerMillionOutputTokens()).isEqualTo(expectedCost.costPerOutputToken()); | |
| * assertThat(usage.getServiceType()).isEqualTo(expectedCost.pipeline()); | |
| * } | |
| * List<LLMTokenUsage> returnedTokenUsages = llmTokenUsageService.saveLLMTokenUsage( | |
| * builder -> builder.withMessage(irisMessage).withExercise(exercise).withUser(irisSession.getUser()).withCourse(course).withTokens(tokens)); | |
| * assertThat(returnedTokenUsages).hasSize(5); | |
| * for (int i = 0; i < returnedTokenUsages.size(); i++) { | |
| * LLMTokenUsage usage = returnedTokenUsages.get(i); | |
| * PyrisLLMCostDTO expectedCost = tokens.get(i); | |
| * assertThat(usage.getModel()).isEqualTo(expectedCost.modelInfo()); | |
| * assertThat(usage.getNumInputTokens()).isEqualTo(expectedCost.numInputTokens()); | |
| * assertThat(usage.getNumOutputTokens()).isEqualTo(expectedCost.numOutputTokens()); | |
| * assertThat(usage.getCostPerMillionInputTokens()).isEqualTo(expectedCost.costPerInputToken()); | |
| * assertThat(usage.getCostPerMillionOutputTokens()).isEqualTo(expectedCost.costPerOutputToken()); | |
| * assertThat(usage.getServiceType()).isEqualTo(expectedCost.pipeline()); | |
| * } |
| * private List<LLMRequest> getMockLLMCosts() { | ||
| * List<LLMRequest> costs = new ArrayList<>(); | ||
| * for (int i = 0; i < 5; i++) { | ||
| * costs.add(new LLMRequest("test-llm", i * 10 + 5, i * 0.5f, i * 3 + 5, i * 0.12f, "IRIS_CHAT_EXERCISE_MESSAGE")); | ||
| * } | ||
| * return costs; | ||
| * } |
There was a problem hiding this comment.
Use Consistent Data Types for Token Cost Objects
The method getMockLLMCosts() currently returns a List<LLMRequest>, but the tokens are expected to be of type PyrisLLMCostDTO based on their usage elsewhere in the code. For consistency and to prevent potential type mismatch issues, update the method to use PyrisLLMCostDTO.
Apply this diff to update the method:
- private List<LLMRequest> getMockLLMCosts() {
- List<LLMRequest> costs = new ArrayList<>();
+ private List<PyrisLLMCostDTO> getMockLLMCosts() {
+ List<PyrisLLMCostDTO> costs = new ArrayList<>();
for (int i = 0; i < 5; i++) {
- costs.add(new LLMRequest("test-llm", i * 10 + 5, i * 0.5f, i * 3 + 5, i * 0.12f, "IRIS_CHAT_EXERCISE_MESSAGE"));
+ costs.add(new PyrisLLMCostDTO("test-llm", i * 10 + 5, i * 0.5f, i * 3 + 5, i * 0.12f, LLMServiceType.IRIS_CHAT_EXERCISE_MESSAGE.toString()));
}
return costs;
}This change aligns the data types and ensures that you're using the appropriate DTO for token cost information.
📝 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.
| * private List<LLMRequest> getMockLLMCosts() { | |
| * List<LLMRequest> costs = new ArrayList<>(); | |
| * for (int i = 0; i < 5; i++) { | |
| * costs.add(new LLMRequest("test-llm", i * 10 + 5, i * 0.5f, i * 3 + 5, i * 0.12f, "IRIS_CHAT_EXERCISE_MESSAGE")); | |
| * } | |
| * return costs; | |
| * } | |
| private List<PyrisLLMCostDTO> getMockLLMCosts() { | |
| List<PyrisLLMCostDTO> costs = new ArrayList<>(); | |
| for (int i = 0; i < 5; i++) { | |
| costs.add(new PyrisLLMCostDTO("test-llm", i * 10 + 5, i * 0.5f, i * 3 + 5, i * 0.12f, LLMServiceType.IRIS_CHAT_EXERCISE_MESSAGE.toString())); | |
| } | |
| return costs; | |
| } |
Checklist
General
Server
Motivation and Context
This PR fixes parts of an open Feature request: Issue
The feature request also includes a graphical interface, but this will be implemented later due to the beginning of the lectures next week.
Description
I worked with Pyris and Artemis to implement the requested feature. Pyris now sends tokens with a status message if tokens were used. Those tokens are of a new Type called
PyrisLLMCostDTOthat saves input and output tokens, the used LLM and also the service that used those tokens:If Artemis receives those tokens it saves them using the
LLMTokenUsageServicein a new table called llm_token_usage. This table contains all the information related to the LLM call: model, number and cost of input and output tokens, the course, exercise, user and IrisMessage Id and a traceId. The traceId is a unique Id that is used to group calls that were made for one message, for Iris it is the jobId as every Pipeline called for one Job has the same JobId.Steps for Testing
Prerequisites:
The new
llm_token_usagetable is migrated, and the Pyris is running with the branch feature/track-usage-of-iris-requests (related PR)Iris is activated for the exercise and the course
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Test Coverage
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation