Model Export to liteRT #2405
Model Export to liteRT #2405
Conversation
This reverts commit 62d2484.
This reverts commit de830b1.
Refactored exporter and registry logic for better type safety and error handling. Improved input signature methods in config classes by extracting sequence length logic. Enhanced LiteRT exporter with clearer verbose handling and stricter error reporting. Registry now conditionally registers LiteRT exporter and extends export method only if dependencies are available.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @pctablet505, 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 introduces a comprehensive and extensible framework for exporting Keras-Hub models to various formats, with an initial focus on LiteRT. The system is designed to seamlessly integrate with Keras-Hub's model architecture, particularly by addressing the unique challenge of handling dictionary-based model inputs during the export process. This enhancement significantly improves the deployability of Keras-Hub models by providing a standardized and robust export pipeline, alongside crucial compatibility fixes for TensorFlow's SavedModel/TFLite export mechanisms.
Highlights
- New Model Export Framework: Introduced a new, extensible framework for exporting Keras-Hub models, designed to support various formats and model types.
- LiteRT Export Support: Added specific support for exporting Keras-Hub models to the LiteRT format, verified for models like gemma3, llama3.2, and gpt2.
- Registry-Based Configuration: Implemented an
ExporterRegistryto manage and retrieve appropriate exporter configurations and exporters based on model type and target format. - Input Handling for Keras-Hub Models: Developed a
KerasHubModelWrapperto seamlessly convert Keras-Hub's dictionary-based inputs to the list-based inputs expected by the underlying Keras LiteRT exporter. - TensorFlow Export Compatibility: Added compatibility shims (
_get_save_specand_trackable_children) to Keras-HubBackbonemodels to ensure proper functioning with TensorFlow's SavedModel and TFLite export utilities. - Automated Export Method Extension: The
Taskclass in Keras-Hub models is now automatically extended with anexportmethod, simplifying the model export process for users.
Using Gemini Code Assist
The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature: model exporting to liteRT. The implementation is well-structured, using a modular and extensible registry pattern. However, there are several areas that require attention. The most critical issue is the complete absence of tests for the new export functionality, which is a direct violation of the repository's style guide stating that testing is non-negotiable. Additionally, I've identified a critical bug in the error handling logic within the lite_rt.py exporter that includes unreachable code. There are also several violations of the style guide regarding the use of type hints in function signatures across all new files. I've provided specific comments and suggestions to address these points, which should help improve the robustness, maintainability, and compliance of this new feature.
keras_hub/src/export/lite_rt.py
Outdated
| try: | ||
| # Export using the Keras exporter | ||
| keras_exporter.export(filepath) | ||
|
|
||
| if self.verbose: | ||
| print(f"Export completed successfully to: {filepath}.tflite") | ||
|
|
||
| except Exception as e: | ||
| raise RuntimeError(f"LiteRT export failed: {e}") from e | ||
| keras_exporter.export(filepath) | ||
|
|
||
| if self.verbose: | ||
| print(f"✅ Export completed successfully!") | ||
| print(f"📁 Model saved to: {filepath}.tflite") | ||
|
|
||
| except Exception as e: | ||
| if self.verbose: | ||
| print(f"❌ Export failed: {e}") | ||
| raise |
There was a problem hiding this comment.
The try...except block for the export process contains a bug. The code from line 102 is unreachable due to the raise statement on line 101. Additionally, having two consecutive except Exception as e: blocks is a syntax error. The error handling logic should be corrected to properly handle exceptions and report success or failure.
| try: | |
| # Export using the Keras exporter | |
| keras_exporter.export(filepath) | |
| if self.verbose: | |
| print(f"Export completed successfully to: {filepath}.tflite") | |
| except Exception as e: | |
| raise RuntimeError(f"LiteRT export failed: {e}") from e | |
| keras_exporter.export(filepath) | |
| if self.verbose: | |
| print(f"✅ Export completed successfully!") | |
| print(f"📁 Model saved to: {filepath}.tflite") | |
| except Exception as e: | |
| if self.verbose: | |
| print(f"❌ Export failed: {e}") | |
| raise | |
| try: | |
| # Export using the Keras exporter | |
| keras_exporter.export(filepath) | |
| if self.verbose: | |
| print(f"Export completed successfully to: {filepath}.tflite") | |
| except Exception as e: | |
| if self.verbose: | |
| print(f"❌ Export failed: {e}") | |
| raise RuntimeError(f"LiteRT export failed: {e}") from e |
keras_hub/src/export/base.py
Outdated
| ) | ||
|
|
||
| @abstractmethod | ||
| def _is_model_compatible(self) -> bool: |
There was a problem hiding this comment.
The style guide specifies that type hints should not be used in function signatures 1. Instead, type information should be provided in the Args section of the docstring. This rule is violated throughout the new files. For example, _is_model_compatible(self) -> bool:. Please remove type hints from all function signatures in this file and ensure the types are documented in the docstrings.
Style Guide References
Footnotes
-
KerasHub does not use type hints in function signatures. Type information should be provided in the docstring. ↩
keras_hub/src/export/configs.py
Outdated
| EXPECTED_INPUTS = ["token_ids", "padding_mask"] | ||
| DEFAULT_SEQUENCE_LENGTH = 128 | ||
|
|
||
| def _is_model_compatible(self) -> bool: |
There was a problem hiding this comment.
Similar to other new files, this file uses type hints in function signatures (e.g., _is_model_compatible(self) -> bool:), which is against the style guide 1. Please remove the type hints from function signatures and move the type information to the docstrings.
Style Guide References
Footnotes
-
KerasHub does not use type hints in function signatures. Type information should be provided in the docstring. ↩
keras_hub/src/export/configs.py
Outdated
| def _get_sequence_length(self) -> int: | ||
| """Get sequence length from model or use default.""" | ||
| if hasattr(self.model, 'preprocessor') and self.model.preprocessor: | ||
| return getattr(self.model.preprocessor, 'sequence_length', self.DEFAULT_SEQUENCE_LENGTH) | ||
| return self.DEFAULT_SEQUENCE_LENGTH |
There was a problem hiding this comment.
The _get_sequence_length method is duplicated across CausalLMExporterConfig, TextClassifierExporterConfig, Seq2SeqLMExporterConfig, and TextModelExporterConfig. To improve maintainability and reduce code duplication, this method should be moved to the base class KerasHubExporterConfig in keras_hub/src/export/base.py.
keras_hub/src/export/lite_rt.py
Outdated
| def __init__(self, config: KerasHubExporterConfig, | ||
| max_sequence_length: Optional[int] = None, | ||
| aot_compile_targets: Optional[list] = None, | ||
| verbose: bool = False, | ||
| **kwargs): |
There was a problem hiding this comment.
This file uses type hints in function signatures (e.g., __init__(self, config: KerasHubExporterConfig, ...)), which is against the style guide 1. Please remove the type hints and move the type information to the docstrings.
Style Guide References
Footnotes
-
KerasHub does not use type hints in function signatures. Type information should be provided in the docstring. ↩
keras_hub/src/export/registry.py
Outdated
| pass | ||
|
|
||
|
|
||
| def export_model(model, filepath: str, format: str = "lite_rt", **kwargs): |
There was a problem hiding this comment.
This file uses type hints in function signatures (e.g., export_model(model, filepath: str, ...)), which is against the style guide 1. Please remove the type hints and move the type information to the docstrings.
Style Guide References
Footnotes
-
KerasHub does not use type hints in function signatures. Type information should be provided in the docstring. ↩
keras_hub/src/export/registry.py
Outdated
| **kwargs: Additional arguments passed to the exporter | ||
| """ | ||
| # Ensure registry is initialized | ||
| initialize_export_registry() |
There was a problem hiding this comment.
The initialize_export_registry() function is called at the module level on line 125, ensuring it runs once upon import. Calling it again within the export_model function is redundant and can be removed for minor performance improvement and cleaner code.
keras_hub/src/models/__init__.py
Outdated
| except ImportError as e: | ||
| print(f"⚠️ Failed to import Keras-Hub export functionality: {e}") |
There was a problem hiding this comment.
Using print for warnings is not ideal as it doesn't give developers control over the message. It's better practice to use warnings.warn to log this failure. This allows users of the library to filter or redirect warnings as needed.
| except ImportError as e: | |
| print(f"⚠️ Failed to import Keras-Hub export functionality: {e}") | |
| except ImportError as e: | |
| import warnings | |
| warnings.warn(f"⚠️ Failed to import Keras-Hub export functionality: {e}") |
Introduces the keras_hub.api.export submodule and updates the main API to expose it. The new export module imports various exporter configs and functions from the internal export package, making them available through the public API.
Added ImageClassifierExporterConfig, ImageSegmenterExporterConfig, and ObjectDetectorExporterConfig to the export API. Improved input shape inference and dummy input generation for image-related exporter configs. Refactored LiteRTExporter to better handle model type checks and input signature logic, with improved error handling for input mapping.
Moved the 'import keras' statement to the top of the module and removed redundant local imports within class methods. This improves code clarity and avoids repeated imports.
Deleted the debug_object_detection.py script, which was used for testing object detection model outputs and export issues. This cleanup removes unused debugging code from the repository.
Renames all references of 'LiteRT' to 'Litert' across the codebase, including file names, class names, and function names. Updates exporter registry and API imports to use the new 'litert' naming. Also improves image model exporter configs to dynamically determine input dtype from the model, enhancing flexibility for different input types. Adds support for ImageSegmenter model type detection in the exporter registry.
Refactored InputSpec definitions in exporter configs for improved readability by placing each argument on a separate line. Updated import path in litert.py to import from keras.src.export.litert instead of keras.src.export.litert_exporter.
|
@pctablet505 can you update the Colab to use the changes from this PR? and keep the demo short where you load a model and export it, and reload and verify numerics? |
|
Also if you have addressed the Gemini comments, please respond on it. Is this PR dependent on the PR in Keras repo? - keras-team/keras#21674 |
Simplifies and unifies input signature and dummy input generation for text and image models by removing redundant helper methods and centralizing logic. Updates model building in KerasHubExporter to use input signatures and improves error handling. Refactors LiteRT exporter to use the new parameterized input signature and model building approach, reducing code duplication and improving maintainability.
Removed redundant registry initialization in export_model and clarified model building comments in KerasHubExporter. Switched to using warnings.warn for import errors in models/__init__.py instead of print statements for better error reporting.
|
@gemini-code-assist please review it again |
There was a problem hiding this comment.
Code Review
This is a significant and valuable addition to KerasHub, enabling model exportation to LiteRT. The overall structure is well-designed, following a modular and extensible pattern similar to Hugging Face's Optimum.
I've reviewed this pull request, focusing on correctness, maintainability, and adherence to the repository's style guide. The style guide was instrumental in this review, particularly the sections on Testing and Validation (lines 401-496) and Docstrings and Type Hinting (lines 364-398).
My main findings are:
-
Critical Issue: Missing Tests
- The pull request introduces substantial new functionality for model exporting across several new files (
base.py,configs.py,litert.py,registry.py), but it lacks corresponding tests. - The repository style guide is explicit that "Testing is a non-negotiable part of every contribution" (line 403) and "Every .py file containing logic...must have a corresponding
_test.pyfile" (line 406). - Please add comprehensive unit tests for the new export logic, covering different model types, configurations, and edge cases. This is crucial to ensure the robustness and correctness of this feature.
- The pull request introduces substantial new functionality for model exporting across several new files (
-
Other Findings
- I've also left several inline comments regarding a bug in model type detection, incorrect dtype handling, and violations of the docstring style guide. Please address these to improve code quality and consistency.
| if hasattr(self.model, "inputs") and self.model.inputs: | ||
| dtype = str(self.model.inputs[0].dtype) |
There was a problem hiding this comment.
There's a potential bug here in how the dtype is being retrieved. str(self.model.inputs[0].dtype) will likely produce a string representation like '<dtype: 'float32'>' which is not a valid dtype string for keras.layers.InputSpec.
To correctly get the dtype name, you should use self.model.inputs[0].dtype.name.
This issue also exists in ObjectDetectorExporterConfig (line 396) and ImageSegmenterExporterConfig (line 509).
| if hasattr(self.model, "inputs") and self.model.inputs: | |
| dtype = str(self.model.inputs[0].dtype) | |
| if hasattr(self.model, "inputs") and self.model.inputs: | |
| dtype = self.model.inputs[0].dtype.name |
| keras_hub_model_names = [ | ||
| "CausalLM", | ||
| "Seq2SeqLM", | ||
| "TextClassifier", | ||
| "ImageClassifier", | ||
| ] |
There was a problem hiding this comment.
The list of keras_hub_model_names used for model type detection is incomplete. It's missing ObjectDetector and ImageSegmenter, which are defined as exportable model types in configs.py.
This will cause _is_keras_hub_model() to return False for these model types, preventing the custom litert export logic from being used for them. Please add the missing model names to the list.
| keras_hub_model_names = [ | |
| "CausalLM", | |
| "Seq2SeqLM", | |
| "TextClassifier", | |
| "ImageClassifier", | |
| ] | |
| keras_hub_model_names = [ | |
| "CausalLM", | |
| "Seq2SeqLM", | |
| "TextClassifier", | |
| "ImageClassifier", | |
| "ObjectDetector", | |
| "ImageSegmenter", | |
| ] |
| def __init__(self, model, **kwargs): | ||
| """Initialize the exporter configuration. | ||
| Args: | ||
| model: The Keras-Hub model to export | ||
| **kwargs: Additional configuration parameters | ||
| """ |
There was a problem hiding this comment.
The docstrings for __init__ methods throughout the new export module do not fully adhere to the repository's style guide. According to the style guide (Repository Style Guide, lines 373-375), type information should be provided in the Args section in the format arg_name: type. description.
For example, here the model argument is missing its type. It should be:
Args:
model: keras.Model. The Keras-Hub model to export.
**kwargs: Additional configuration parameters.This issue is present in most new docstrings in keras_hub/src/export/base.py, keras_hub/src/export/configs.py, and keras_hub/src/export/litert.py. Please update them to be consistent with the style guide.
"""Initialize the exporter configuration.
Args:
model: keras.Model. The Keras-Hub model to export.
**kwargs: Additional configuration parameters.
"""| Returns: | ||
| Dict[str, Any]: Dictionary mapping input names to their signatures | ||
| """ |
There was a problem hiding this comment.
The format for the Returns section in the docstring doesn't align with the example provided in the style guide (Repository Style Guide, lines 386-388). The guide suggests a descriptive sentence, e.g., A dictionary mapping tokens to their integer IDs..
Using Python type hints like Dict[str, Any] is discouraged in docstrings by the style guide. Please rephrase this to be more descriptive.
For example:
Returns:
A dictionary mapping input names to their tensor specifications.
This applies to other Returns sections in the new files as well.
Returns:
A dictionary mapping input names to their signatures.| wrapped_model, | ||
| input_signature=input_signature, | ||
| aot_compile_targets=self.aot_compile_targets, | ||
| verbose=1 if self.verbose else 0, |
There was a problem hiding this comment.
This is a minor suggestion for code style. Using int(self.verbose) is a more Pythonic way to convert a boolean to an integer (0 or 1) compared to the ternary operator 1 if self.verbose else 0.
| verbose=1 if self.verbose else 0, | |
| verbose=int(self.verbose), |
Added support for model export, currently able to convert gemma3, llama3.2, gpt2 models, and verified numerics also.
Colab Notebook
(https://colab.research.google.com/gist/pctablet505/45a48c42fa91cc27995cdaefda57cb28/model-export.ipynb)