Conversation
lethargicpanda
requested review from
JolandaVerhoef,
calren and
ksemenova
as code owners
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates a new 'Gemini Hybrid' sample into the AI Sample Catalog. The sample is designed to illustrate how to leverage both on-device and cloud-based Gemini models for generative AI tasks, specifically for generating and translating hotel reviews. It provides a practical example of implementing a robust AI solution that can adapt to different device capabilities and network conditions by intelligently switching between local and remote inference. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'Gemini Hybrid' sample, demonstrating a hybrid approach with on-device and cloud-based models. The implementation is well-structured within the new module, utilizing a ViewModel and Jetpack Compose for the UI.
My review focuses on adherence to the repository's architecture guidelines, code consistency, and maintainability. Key feedback includes suggestions to align with the recommended layered architecture, improve composable function signatures for better reusability, and correct some inconsistencies in the build configuration and documentation. Addressing these points will enhance the quality and clarity of the new sample.
| The application first attempts to perform a task (e.g., summarization) using the on-device Gemini Nano model through the ML Kit GenAI API. If the model is not supported on the device or fails to download, it seamlessly falls back to the Gemini Flash model in the cloud using the Firebase AI SDK. | ||
|
|
||
| ### Key Snippets | ||
|
|
||
| #### On-Device Inference (ML Kit) | ||
| ```kotlin | ||
| val summarizer = Summarization.getClient(options) | ||
| val featureStatus = summarizer.checkFeatureStatus().await() | ||
| if (featureStatus == FeatureStatus.READY) { | ||
| summarizer.runInference(request) { ... }.await() | ||
| } | ||
| ``` | ||
|
|
||
| #### Cloud Inference (Firebase AI) | ||
| ```kotlin | ||
| val generativeModel = Firebase.ai.generativeModel("gemini-1.5-flash") | ||
| val response = generativeModel.generateContent(prompt) | ||
| ``` |
There was a problem hiding this comment.
The description and code snippets in this README do not match the actual implementation in the sample. The README describes using the ML Kit GenAI API (Summarization.getClient), but the code uses the Firebase AI Hybrid SDK (Firebase.ai.generativeModel).
This discrepancy can be very confusing for developers trying to understand the sample. Please update the README to accurately reflect the code's implementation and provide correct code snippets.
This is related to the style guide recommendation to keep documentation aligned with the code.
References
- When a view model or the business logic is modified in the code of a sample, verify that these changes are properly reflected in the README.md of this sample. (link)
| _selectedLanguage.value = language | ||
| } | ||
|
|
||
| fun generateReview(tagStrings: List<String>) { |
There was a problem hiding this comment.
The business logic for interacting with the Firebase AI SDK is implemented directly within the GeminiHybridViewModel. The repository's architecture guidelines strongly recommend separating concerns by placing data-sourcing logic in a dedicated data layer, exposed to the ViewModel via a repository.
Refactoring this logic into a GeminiHybridRepository would better align with the recommended architecture, improving testability and maintainability.
References
- The data layer should expose data via a repository. UI components (composables, activities, ViewModels) should not interact directly with data sources (databases, DataStore, network, etc.). (link)
|
|
||
| android { | ||
| namespace = "com.android.ai.samples.geminihybrid" | ||
| compileSdk = 35 |
There was a problem hiding this comment.
The compileSdk version is set to 35, which is inconsistent with the main app module that uses version 36. To ensure consistency across the project and avoid potential build issues, it's recommended to use the same compileSdk version.
compileSdk = 36| implementation(libs.genai.summarization) | ||
| implementation(libs.kotlinx.coroutines.guava) |
There was a problem hiding this comment.
These dependencies (genai.summarization and kotlinx.coroutines.guava) appear to be unused in the gemini-hybrid module. The implementation uses the Firebase AI SDK directly, not the ML Kit Summarization API. To keep the module clean and avoid unnecessary dependencies, these lines should be removed.
|
|
||
| @OptIn(ExperimentalLayoutApi::class) | ||
| @Composable | ||
| fun InitialReviewUi( |
There was a problem hiding this comment.
The composable function InitialReviewUi is missing a Modifier parameter. According to the architecture guidelines, all composables (except top-level screens) should accept a Modifier as the first optional parameter. This improves flexibility, allowing the caller to customize layout, appearance, and behavior.
This applies to other composables in this file as well: GeneratingUi, SuccessReviewUi, ErrorUi, LanguageDropdown, InferenceModeDropdown, and StatusText.
| fun InitialReviewUi( | |
| fun InitialReviewUi( | |
| modifier: Modifier = Modifier, |
References
- Every composable function (except top level screen composable) should take a
Modifieras a parameter with a default value. It should be positionned as the first optional parameter. (link)
| _uiState.update { state -> | ||
| if (state is GeminiHybridUiState.Generating) { | ||
| state.copy( | ||
| isCloud = isCloud, | ||
| partialOutput = state.partialOutput + (chunk.text ?: "") | ||
| ) | ||
| } else { | ||
| GeminiHybridUiState.Generating( | ||
| isCloud = isCloud, | ||
| partialOutput = chunk.text ?: "", | ||
| isTranslation = false | ||
| ) | ||
| } |
There was a problem hiding this comment.
The logic for updating the UI state while the content stream is being collected is duplicated in generateReview and translate (lines 193-205). This makes the code harder to maintain.
Consider extracting this state update logic into a private helper function to reduce duplication and improve readability.
lethargicpanda
force-pushed
the
te/hybrid-review
branch
from
ce1e4d3 to
d6a1161
Compare
1 participant
No description provided.