Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Local LLM Backend #116

Merged
merged 10 commits into from
May 9, 2023
Merged

Local LLM Backend #116

merged 10 commits into from
May 9, 2023

Conversation

danforbes
Copy link
Contributor

This seems to work in simple cases, but it's a very naive implementation and I'm happy to accept any suggestions to improve it.

Note that this is using a feature branch of the llm crate rustformers/llm@main...danforbes:llama-rs:dfo/chore/llm-chain

cc: @philpax

Copy link
Contributor

@williamhogman williamhogman left a comment

Choose a reason for hiding this comment

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

Looks really good. Could merge this as an initial version :)

}
}

pub fn load<M: llm::KnownModel + 'static>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this one needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which one? The lifetime? If I remove the lifetime, it breaks and says the parameter M may not live long enough...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah meant the whole function :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, it doesn't need to be published, but it is currently being used.

))?;

let params = invocation_options.unwrap_or_default().into();
let llm: Box<dyn llm::Model> = match model_type.as_str() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be done upstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you mean exposing a load option that takes an enum variant referring to the model type?

Copy link
Contributor

Choose a reason for hiding this comment

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

meant the mapping of strings to model types :)

llm-chain-local/src/executor.rs Outdated Show resolved Hide resolved
#[derive(Debug, Clone, Serialize, Deserialize, Default)]
pub struct PerExecutor {
/// Optional path to the LLM.
pub model_path: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be a Path instead perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without trying too hard, a first attempt fails because Path cannot be Sized, which appears to be required for Option.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes. How about PathBuf. Should be Sized.

/// Optional path to the LLM.
pub model_path: Option<String>,
/// Optional type (e.g. LLaMA, GPT-Neo-X) of the LLM.
pub model_type: Option<String>,
Copy link
Contributor

@drager drager May 6, 2023

Choose a reason for hiding this comment

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

No ideas to have an enum and a fallback? To let know what models are supported?

I mean something like:

enum ModelType {
  LLama,
  GPTNeoX,
  Custom(String)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The model architecture enum isn't currently serializable (although I don't see why it can't be), so I'm leaving this a string for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah alright! I see!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hopefully this will be changed soon rustformers/llm#200

@danforbes danforbes marked this pull request as draft May 6, 2023 19:43
@danforbes danforbes force-pushed the dfo/feat/local-llm branch from 36b2b23 to 9c05a5d Compare May 9, 2023 16:59
@danforbes danforbes force-pushed the dfo/feat/local-llm branch from 9c05a5d to ccf9051 Compare May 9, 2023 17:07
@danforbes danforbes marked this pull request as ready for review May 9, 2023 17:07
@williamhogman williamhogman merged commit cb8c4f3 into sobelio:main May 9, 2023
@danforbes danforbes deleted the dfo/feat/local-llm branch May 9, 2023 20:49
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.

3 participants