Refactor: Introduce Modular Architecture for llama-model.cpp#323
Open
devin-ai-integration[bot] wants to merge 1 commit intomasterfrom
Open
Refactor: Introduce Modular Architecture for llama-model.cpp#323devin-ai-integration[bot] wants to merge 1 commit intomasterfrom
devin-ai-integration[bot] wants to merge 1 commit intomasterfrom
Conversation
This commit introduces a modular architecture to refactor the monolithic src/llama-model.cpp (20,498 lines) into a maintainable structure. Key changes: - Created src/llama-model/ directory structure with subdirectories: - architectures/: One file per architecture (starting with LLAMA) - interfaces/: Interface definitions (IModelLoader, IGraphBuilder) - registry/: Architecture registration system - base/, loading/, memory/: Future modular components - Implemented ArchitectureRegistry pattern: - Singleton registry for architecture builders - Factory-based lazy instantiation - REGISTER_ARCHITECTURE macro for easy registration - Created comprehensive documentation: - docs/development/REFACTORING-GUIDE.md with migration guide - Inline documentation in all new interfaces - Examples and common patterns - Updated build system: - Modified src/CMakeLists.txt to include modular sources - Uses GLOB to automatically include architecture files - Updated CODEOWNERS: - Added ownership for new modular directories - Maintained existing ownership patterns Backwards compatibility: - All existing APIs remain unchanged - Loading flow in src/llama.cpp unchanged - All 37 existing tests pass (100% pass rate) - No breaking changes to public API This is Phase 1 of the refactoring. Future phases will: - Extract remaining 88+ architectures - Implement loading/memory modules - Add comprehensive unit tests - Remove deprecated code paths Related: #<issue_number> (if applicable) Co-Authored-By: Jake Cosme <jake@cognition.ai>
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Make sure to read the contributing guidelines before submitting a PR
Summary
This PR introduces the foundational infrastructure for refactoring
src/llama-model.cpp(20,498 lines) into a modular architecture. This is Phase 1 only - it creates the scaffolding and patterns but does not yet migrate any existing code.Link to Devin run: https://app.devin.ai/sessions/a329f05a4cdc4f2eb254bcb7ee3fb2ac
Requested by: Jake Cosme (@jakexcosme)
Changes
New Infrastructure
Directory structure under
src/llama-model/:architectures/- Will contain one file per architecture (currently has LLAMA example)interfaces/- Interface definitions (IModelLoader,IGraphBuilder,ArchitectureBuilder)registry/- Architecture registration system with singleton patternbase/,loading/,memory/- Empty directories for future modulesRegistry Pattern:
ArchitectureRegistrywith factory-based lazy instantiationExample Implementation:
LlamaArchitectureBuilderthat demonstrates the pattern (currently just delegates to existing code)Documentation: Comprehensive 518-line
REFACTORING-GUIDE.mdwith examples and migration strategyBuild System
src/CMakeLists.txtto usefile(GLOB ...)for automatic inclusion of architecture filesCODEOWNERSfor new directory structureBackwards Compatibility
✅ All existing APIs unchanged - No modifications to
llama_modelinterface✅ All 37 tests pass (100% pass rate)
✅ No functional changes - New code is not yet integrated into existing flow
Important Notes for Reviewers
This PR Does NOT:
llama-model.cppThis PR DOES:
Key Review Points
Architecture Design: Are the interfaces (
ArchitectureBuilder,ArchitectureRegistry) well-designed for the intended refactoring?Documentation Quality: Is
docs/development/REFACTORING-GUIDE.mdclear and comprehensive enough for contributors to follow?Build System: The use of
file(GLOB ...)is convenient but requires manual CMake re-runs when files are added. Is this acceptable?Dead Code: The new registry and LLAMA builder are not yet called from anywhere. Is it okay to merge infrastructure before integration?
Empty Directories: Several directories (
base/,loading/,memory/) are created but empty. Should these be added in later PRs instead?Macro Safety: The
REGISTER_ARCHITECTUREmacro uses__LINE__for uniqueness. Review for potential edge cases.Testing
Next Steps (Phase 2)
After this PR is merged, Phase 2 will:
llama_model::build_graph()Checklist
Unit tests for registry(in gitignored directory, will add in Phase 2)Integration with existing code(Phase 2)Migration of remaining architectures(Phase 2+)