-
Notifications
You must be signed in to change notification settings - Fork 525
[Android] New config API for Llm init and generate #10345
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/10345
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New FailureAs of commit d689bc7 with merge base 77c48f7 ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new configuration API for LLM initialization and generation by adding two new config classes and making minor documentation improvements.
- Added LlmModuleConfig with a builder pattern for LLM initialization
- Added LlmGenerationConfig for text generation parameter configuration
- Updated JavaDoc formatting in LlmModule and LlmCallback for clarity
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
extension/android/executorch_android/src/main/java/org/pytorch/executorch/extension/llm/LlmModuleConfig.java | Introduces configuration settings for LLM initialization via a fluent builder |
extension/android/executorch_android/src/main/java/org/pytorch/executorch/extension/llm/LlmModule.java | Minor JavaDoc formatting improvements |
extension/android/executorch_android/src/main/java/org/pytorch/executorch/extension/llm/LlmGenerationConfig.java | Adds settings for controlling text generation parameters using a builder |
extension/android/executorch_android/src/main/java/org/pytorch/executorch/extension/llm/LlmCallback.java | Updates JavaDoc formatting for clarity |
* @throws IllegalArgumentException if required fields are missing | ||
*/ | ||
public LlmModuleConfig build() { | ||
if (modulePath == null || tokenizerPath == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The builder validates that modulePath is required, but there is no builder method to set modulePath. Consider adding a method like 'public Builder modulePath(String modulePath)' to allow the module path to be set.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new configuration API for initializing LLM modules and controlling text generation parameters in the Android extension, following a fluent builder pattern similar to GenerationConfig.
- Added LlmModuleConfig with builder methods for setting up module parameters.
- Added LlmGenerationConfig to configure text generation behaviors.
- Improved comment formatting in LlmModule and LlmCallback for clearer documentation.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
File | Description |
---|---|
LlmModuleConfig.java | Introduces builder-based configuration for LLM module initialization. |
LlmModule.java | Minor comment formatting update for conciseness. |
LlmGenerationConfig.java | Provides a builder pattern configuration for text generation parameters. |
LlmCallback.java | Updates comment formatting to improve clarity regarding JSON statistic definitions. |
@kirklandsign has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Add a config for init, and generate() similar to GenerationConfig