Skip to content

Conversation

@pwilkin
Copy link
Collaborator

@pwilkin pwilkin commented Sep 25, 2025

@ggerganov I know you said you were planning to do it, but honestly it's been a nightmare working on all the model implementations with the huge llama-model.cpp, so I wanted to just get the "easy" albeit tedious part out of the way. Moved all llm_build_* definitions to their separate class files in src/models/

@CISC
Copy link
Collaborator

CISC commented Sep 25, 2025

This is a nightmare to review and rebase until merged though, also you seem to have pushed the same changes to your Qwen3-Next PR?

@pwilkin
Copy link
Collaborator Author

pwilkin commented Sep 25, 2025

@CISC Ye, need them for working there, but I'll revert once I'm done (unless this is done first and I can merge on top).

I know it's a nightmare, I already kind of went through it when I asked an LLM to automate some tasks and it proceeded merrily ripping out methods just because some classes didn't inherit from llm_graph_context :>

If you want, I can write a script that runs tree-sitter on the original definitions in llama-model.cpp vs the new classes and shows any differences to verify that nothing was accidentally lost.

@jacekpoplawski
Copy link
Contributor

Maybe it would be easier to refactor that partially, just a subset of the models?

@ngxson ngxson mentioned this pull request Sep 29, 2025
@ngxson
Copy link
Collaborator

ngxson commented Sep 29, 2025

This seems to be a good change, just have some other ideas:

  1. I think for now all models can share one single .h file. The main .cpp implementation can be split into smaller files as proposed here
  2. Our naming convention uses - instead of _. Maybe just src/models/(model-name).cpp is enough, no need the llm_build prefix. For example: src/models/gpt2.cpp

@pwilkin
Copy link
Collaborator Author

pwilkin commented Oct 24, 2025

All right, I've done the changes suggested by @ngxson - renamed the files to just .cpp and merged all the header files into one models.h. I've also merged all the current changes from master.

Could we possibly move this forward somehow? This isn't going to get any easier any time soon... @ggerganov ?

@ggerganov
Copy link
Member

Yes, thanks for the help. I will take a look soon at the Qwen 3 Next PR and also this reorganization here. Likely over the weekend.

@ngxson
Copy link
Collaborator

ngxson commented Oct 24, 2025

The diff is too big so I had a look at some files under your fork instead.

It looks good overall, just a nitpick stuff: Seems like many files starts with this pattern:

(maybe one empty line)
#include "models.h"
(some empty line)
...

While it's cleaner to be:

#include "models.h"
(one empty line)
...

Should be a real quick regex replace to change

@pwilkin
Copy link
Collaborator Author

pwilkin commented Oct 24, 2025

@ngxson aye, done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants