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

Suggestion regarding the style of API #45

Open
ddobric opened this issue Aug 1, 2024 · 3 comments
Open

Suggestion regarding the style of API #45

ddobric opened this issue Aug 1, 2024 · 3 comments

Comments

@ddobric
Copy link

ddobric commented Aug 1, 2024

Hi guys,

I love your library—great job. However, I have some suggestions about the API you used in the last package.

Designing the API like this one is not really best way:

var encoder = ModelToEncoder.For("gpt-4o"); //
I suggest using intuitive object names and method names.

The following is widely more acceptably standard in the software industry for many reasons:

`var encoder = TokenEncoder.Create("gpt-4o");

or

var encoder = new TokenEncoder.For("gpt-4o");

You can also use the Tiktoken or Tiiktoken encoder. ModelEncoder is not an intuitive name. We are not encoding Models. Tokenizer is not a concept of model by its original paper. We should stay consistent with the references which we use.

Another suggestion related to providing the "string" ("gpt-xy") is ok, but some kind of enum or similar round string would be helpful.

var encoder = new TokenEncoder.For(Models.Gpt4o);

Hope this helps.

@HavenDV
Copy link
Contributor

HavenDV commented Aug 1, 2024

Hi.
There is a small problem with Encoder.For/Encoder.Create - because this library is split into several packages, and the base package with abstractions for Encoder knows nothing about implementations for it, so I created a new class ModelToEncoder in the meta package, which already combines the main used Encoders. And therefore there is no way to somehow change the TokenEncoder class in it

Maybe I should rename it to Model2Encoder?

@mikethea1
Copy link

@HavenDV how about Encoders.ForModel(...) / Encoders.TryForModel(...)?

@HavenDV
Copy link
Contributor

HavenDV commented Aug 14, 2024

There will be problems because it is part of the namespace.

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

No branches or pull requests

3 participants