Refactor/cleanup model loaders #58
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors model loaders and state field allocators to eliminate code duplication and improve maintainability using Template Method and Abstract Factory design patterns.
Key Changes:
- Introduced
AbstractModelLoaderbase class implementing Template Method pattern, reducing model loader code by ~60% - Created
StateFieldAllocatorabstract factory hierarchy to centralize state field allocation logic, reducing state class code by ~60-66% - Removed AOT (Ahead-of-Time) compilation support and related code
- Added comprehensive architecture documentation (1,229 lines) covering project structure, patterns, and design principles
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| AbstractModelLoader.java | New abstract base class implementing Template Method pattern for model loading workflow |
| StateFieldAllocator.java | New abstract factory for state field allocation with model-specific dimension calculations |
| Qwen3ModelLoader.java | Refactored to extend AbstractModelLoader, implementing model-specific methods |
| Qwen2ModelLoader.java | Refactored to extend AbstractModelLoader, implementing model-specific methods |
| Phi3ModelLoader.java | Refactored to extend AbstractModelLoader, implementing model-specific methods |
| LlamaModelLoader.java | Refactored to extend AbstractModelLoader, implementing model-specific methods |
| MistralModelLoader.java | Refactored to extend AbstractModelLoader, implementing model-specific methods |
| ModelLoader.java | Removed AOT-related code |
| ModelLoadException.java | New exception class for model loading failures |
| Qwen3State.java | Simplified to delegate allocation to Qwen3StateFieldAllocator |
| Qwen2State.java | Simplified to delegate allocation to Qwen2StateFieldAllocator |
| Phi3State.java | Simplified to delegate allocation to Phi3StateFieldAllocator |
| LlamaState.java | Simplified to delegate allocation to LlamaStateFieldAllocator |
| Qwen3StateFieldAllocator.java | New allocator implementing Qwen3-specific dimension calculations |
| Qwen2StateFieldAllocator.java | New allocator implementing Qwen2-specific dimension calculations |
| Phi3StateFieldAllocator.java | New allocator implementing Phi3-specific dimension calculations |
| LlamaStateFieldAllocator.java | New allocator implementing Llama/Mistral dimension calculations |
| AOT.java | Removed AOT compilation support |
| LlamaApp.java | Removed AOT import |
| ARCHITECTURE.md | New comprehensive architecture documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
33
to
35
| protected StateFields createStateFields(Configuration config) { | ||
| StateFieldAllocator allocator = new Qwen3StateFieldAllocator(config, localSize); | ||
| return allocator.allocateFields(); |
There was a problem hiding this comment.
The StateFieldAllocator is created on every call to createStateFields(). Since State objects are reused during inference and createStateFields() is called during initialization, consider caching the allocator instance if createStateFields() could be called multiple times, or document that this is a one-time initialization method.
orionpapadakis
requested changes
Oct 30, 2025
src/main/java/org/beehive/gpullama3/model/loader/AbstractModelLoader.java
Show resolved
Hide resolved
src/main/java/org/beehive/gpullama3/inference/state/StateFieldAllocator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/beehive/gpullama3/inference/state/StateFieldAllocator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/beehive/gpullama3/model/loader/Qwen2ModelLoader.java
Outdated
Show resolved
Hide resolved
src/main/java/org/beehive/gpullama3/model/loader/Qwen2ModelLoader.java
Outdated
Show resolved
Hide resolved
…ity and configuration handling. # Conflicts: # src/main/java/org/beehive/gpullama3/model/loader/Phi3ModelLoader.java # src/main/java/org/beehive/gpullama3/model/loader/Qwen2ModelLoader.java # src/main/java/org/beehive/gpullama3/model/loader/Qwen3ModelLoader.java
orionpapadakis
force-pushed
the
refactor/cleanup_model_loaders
branch
from
27a764b to
8a08dd3
Compare
# Conflicts: # src/main/java/org/beehive/gpullama3/model/loader/Qwen3ModelLoader.java
…ity and configuration handling.
…to corresponding subpackages (fp16 & q8)
orionpapadakis
force-pushed
the
refactor/cleanup_model_loaders
branch
from
8a08dd3 to
eb13ea7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.