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

refactor: from vendor to structural namespaces for models and platforms #68

Merged
merged 1 commit into from
Nov 23, 2024

Conversation

chr-hertel
Copy link
Member

based on the listings of azure, ollama or huggingface the libraries structure/namespaces will scale better over time when not using vendor names like Anthropic or OpenAI as top level namespaces, but rather structural ones like Platform and Model.

I decided against having a tree like structure but splitting into those two namespaces, since it is more like a matrix when looking at some providers supporting multiple different llms.

i'm aware that this will cause larger pain on downstream dependents and might combine it with a further refactoring of Document, Message and Response classes

@chr-hertel chr-hertel self-assigned this Sep 28, 2024
@OskarStark
Copy link
Contributor

The discovery for versions is much worse imho 😟

@DZunke
Copy link
Contributor

DZunke commented Sep 29, 2024

For the bigger picture of supporting more combinations of Provider / Model it looks like an idea. Even it is something very abstract currently for me to see how this improve the library for the future but when looking at the linked pages ... yeah. Good idea.

But why then the namespace Language? Is it meant for Models working with Language Tokens and be open for another namespace for models working with other stuff at a Platform? Just for the further considerations for this decision.

@chr-hertel
Copy link
Member Author

The discovery for versions is much worse imho 😟

There is still autocomplete/discovery possible. You don't have to choose between different Version classes but can use the constants of the class - which can be referenced in yaml as well.

But why then the namespace Language?

It's referring to the Interfaces at first level (LanguageModel vs. EmbeddingsModel) and basically only meant to have a substructure.
Thought about supporting more categories, but would rather extend the catalog and features for those categories before adopting more.

@chr-hertel
Copy link
Member Author

Thanks for the feedback guys 🙏

@chr-hertel
Copy link
Member Author

chr-hertel commented Sep 29, 2024

Maybe as a last addition, my goal would be to bring the ease of using different models, like those platforms (replicate, huggingface, etc) provide, to this lib...
But it's not necessary to merge this now without that feature - might be a larger feature branch

@chr-hertel chr-hertel marked this pull request as draft September 29, 2024 17:39
@chr-hertel chr-hertel force-pushed the refactor-namespaces branch 3 times, most recently from eb8eca5 to bbd9afc Compare October 5, 2024 19:44
@OskarStark
Copy link
Contributor

What I meant with the version class discovery is, that if I end up with one version class containing sonnet and gpt4o for example, what prevents me to setup my LLM Platform OpenAI with Sonnet?

@chr-hertel
Copy link
Member Author

That's not the case here. I switched from additional Version VO to plain string constants in the corresponding model

@chr-hertel
Copy link
Member Author

And also before that the string was not validated and I could have entered whatever in the version object

@OskarStark
Copy link
Contributor

Ok 👍🏻

But now one can (like before) only use a new model when updating the lib 😟

@chr-hertel
Copy link
Member Author

But now one can (like before) only use a new model when updating the lib 😟

That would be a show stopper, true, but i can still use new Gpt($platform, 'my-finetuned-model') or do i miss something?

@chr-hertel
Copy link
Member Author

Hmm, right, i can't indicate the capabilities of the model with Image, Tool or structured output Support ... 🤔

@chr-hertel
Copy link
Member Author

Bottom line: good point 😆 👍

@OskarStark
Copy link
Contributor

OskarStark commented Oct 6, 2024

What about dedicated model classes which know if they support a platform?

You know what I mean? I am currently on a phone an cannot provide a code example

@chr-hertel
Copy link
Member Author

Yup, same here - not really at my desk today. Still thinking tho about the different characteristics per model.

  • Versions
  • Options
  • Capabilities (Tools, Structured Output, Streaming, Vision, etc, ...)
  • MessageBag & Response Conversion

For the first two items we can provide some helpers, like DX gimmicks, but shouldn't be too strict since validation and change scenarios are not ours.
Basically for the last two Change scenarios and validation is not owned by us as well, but the main features of the libs rely on that.
Just writing that down to reflect and think, no argument or action item - only to make those concerns more visible - maybe even in code

@chr-hertel
Copy link
Member Author

With #140 there is a new Model interface that introduces a getVersion + getOptions and in combination the LLM interface that formalizes the capabilities, i think the concerns of this PR are adressed

@chr-hertel chr-hertel marked this pull request as ready for review November 23, 2024 09:34
Copy link

@llamapreview llamapreview bot left a comment

Choose a reason for hiding this comment

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

Auto Pull Request Review from LlamaPReview

Large PR Notification

Dear contributor,

Thank you for your substantial contribution to this project. LlamaPReview has detected that this Pull Request contains a large volume of changes, which exceeds our current processing capacity.

Details:

  • PR and related contents total size: Approximately 69,298 characters
  • Current limit: 50,000 characters

Next steps:

  1. Consider breaking this PR into smaller, more focused changes if possible.
  2. For manual review, please reach out to your team members or maintainers.

We appreciate your understanding and commitment to improving this project. Your contributions are valuable, and we want to ensure they receive the attention they deserve.

LlamaPReview is continuously evolving to better serve the community. Share your thoughts on handling large PRs in our GitHub Discussions - your feedback helps us improve and expand our capabilities.

If you have any questions or need assistance, our community and support team are here to help.

Best regards,
LlamaPReview Team

@chr-hertel chr-hertel merged commit 8a55022 into main Nov 23, 2024
7 checks passed
@chr-hertel chr-hertel deleted the refactor-namespaces branch November 23, 2024 10:17
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