Skip to content
This repository has been archived by the owner on Jun 24, 2024. It is now read-only.

Split up modules #135

Merged
merged 10 commits into from
Apr 13, 2023
Merged

Split up modules #135

merged 10 commits into from
Apr 13, 2023

Conversation

philpax
Copy link
Collaborator

@philpax philpax commented Apr 13, 2023

This is a little overdue I think, and it'll cause problems for the other PRs, but it also makes it much easier to maintain.

I've made a few controversial changes in the last three commits. The rest are pretty straightforward.

@philpax philpax added the meta:maintenance Changes that will make it easier for us to maintain code label Apr 13, 2023
@philpax philpax requested a review from setzer22 April 13, 2023 03:06
Copy link
Collaborator

@setzer22 setzer22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good. I think the way modules are split makes sense 👍

Hopefully this wouldn't cause too many conflicts with existing PRs? e.g. #125 comes to mind

Comment on lines -76 to +75
let (_, vocabulary) = args.model_load.load();
let toks = match vocabulary.tokenize(&prompt, false) {
let model = args.model_load.load();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the change. I had considered doing this a few times before. Since both the model and the vocabulary are meant to be immutable after creation, bundling them into the same struct will hardly cause issues.

Comment on lines +12 to +16
// The size of a scratch buffer used for inference. This is used for temporary
// storage of intermediate results during inference.
//
// The specific value was copied from `llama.cpp`.
const SCRATCH_SIZE: usize = 512 * 1024 * 1024;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think llama.cpp figured out a proper way to compute this value. We should have a look at this. Not in this PR of course 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@philpax
Copy link
Collaborator Author

philpax commented Apr 13, 2023

Hopefully this wouldn't cause too many conflicts with existing PRs? e.g. #125 comes to mind

Yeah, I'm a little concerned about that one myself, but I don't think it should be too bad - it'll mostly just be discarding the changes to loader.rs here. The others shouldn't be too hard to accommodate.

@philpax philpax merged commit 4938dad into rustformers:main Apr 13, 2023
@philpax philpax deleted the split-up-modules branch April 13, 2023 09:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
meta:maintenance Changes that will make it easier for us to maintain code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants