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

Structural Overhaul #162

Merged
merged 40 commits into from
Apr 30, 2023
Merged

Structural Overhaul #162

merged 40 commits into from
Apr 30, 2023

Conversation

danforbes
Copy link
Contributor

Replaces #141. Includes code from #85 and #125.

  • GGML is now a submodule and a single crate
  • Support for GPT-2 architecture
  • Structural refactor into llm, llm-base, model, and llm-cli crates

@philpax
Copy link
Collaborator

philpax commented Apr 30, 2023

Aha, I was just about to reply to the BLOOM PR!

I would suggest keeping BLOOM in - there's no reason to remove that code if someone can fix it with some work (we'll make an issue for it) - but we'll remove it from the list of features, and make a note on the crate itself.

As for the rest, let me have a look...

@@ -1,39 +1,31 @@
# LLaMA-rs

<!-- markdownlint-disable-file MD026 -->
This project is a Rust port of
[llama.cpp](https://github.com/ggerganov/llama.cpp) 🦙🦀🚀
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm thinking we'll remove this wording as we grow to accommodate more LLMs. I'll revise the wording on this after this PR lands, so nothing for you to do here - just mentioning it.

@danforbes
Copy link
Contributor Author

I don't see the value of keeping non-working code in the main branch. I would suggest keeping BLOOM in a feature branch until it's working.

README.md Outdated Show resolved Hide resolved
ggml-rs/Cargo.toml Outdated Show resolved Hide resolved
ggml-rs/build.rs Outdated Show resolved Hide resolved
@hhamud
Copy link
Contributor

hhamud commented Apr 30, 2023

btw could you add me as a git commit author rather than a mention as you did when you upstreamed the ggml changes. @danforbes

@philpax
Copy link
Collaborator

philpax commented Apr 30, 2023

I don't see the value of keeping non-working code in the main branch. I would suggest keeping BLOOM in a feature branch until it's working.

Code that isn't kept in the main branch tends to rot, especially if we start making larger changes. As far as I know, BLOOM is done, we just need to figure out what's causing it to fail generation.

It's much easier for someone to pick it up and fix it if it's present; I think it's also a good way to ensure our abstractions work as expected as we increase the number of models.


Apart from that, PR looks great! Well done :) Looking forward to merging this, I think it's a huge improvement.

@hhamud
Copy link
Contributor

hhamud commented Apr 30, 2023

I don't see the value of keeping non-working code in the main branch. I would suggest keeping BLOOM in a feature branch until it's working.

Code that isn't kept in the main branch tends to rot, especially if we start making larger changes. As far as I know, BLOOM is done, we just need to figure out what's causing it to fail generation.

It's much easier for someone to pick it up and fix it if it's present; I think it's also a good way to ensure our abstractions work as expected as we increase the number of models.

Apart from that, PR looks great! Well done :) Looking forward to merging this, I think it's a huge improvement.

I personally think we should just merge and place a comment on the issue of it not working. I'd pick it up again but I don't have time for it till next week.

danforbes and others added 2 commits April 30, 2023 08:14
Co-authored-by: @hhamud <53880692+hhamud@users.noreply.github.com>
@danforbes
Copy link
Contributor Author

I've added back the BLOOM crate and tagged @hhamud as a co-author ✅

@philpax
Copy link
Collaborator

philpax commented Apr 30, 2023

All that's left are the formatting and restoring bindings generation. Are you OK for me to do that, or do you have changes in the works?

@danforbes
Copy link
Contributor Author

Please go for it - thank you so much 👯

@philpax philpax merged commit 7d650ac into rustformers:main Apr 30, 2023
@danforbes danforbes deleted the dfo/models/gpt2 branch April 30, 2023 21:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants