Skip to content

Add model parameter #11

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add model parameter #11

wants to merge 2 commits into from

Conversation

liugddx
Copy link

@liugddx liugddx commented Jun 13, 2024

No description provided.

gdliu3 added 2 commits June 13, 2024 16:57
@j-dominguez9 j-dominguez9 added the enhancement New feature or request label Jun 28, 2024
@Omaraldarwish
Copy link

Omaraldarwish commented Jul 1, 2024

Might be introducing a bug as same param is used in RecursiveCharacterTextSplitter.from_tiktoken_encoder and OpenAI client call. Checkout tiktoken.model.MODEL_PREFIX_TO_ENCODING and tiktoken.model.MODEL_TO_ENCODING for a supported list of strings to pass.

If this change is happening, might as well make it more general and pass a 'translation_agent_model' class / object which implements get_completion and num_tokens_in_string. This way we give flexibility to user in experimenting, model could be local or api based ... etc.

@methanet
Copy link
Collaborator

methanet commented Jul 1, 2024

Thank you @liugddx for submitting the PR, and @Omaraldarwish for the extension proposal.

@Omaraldarwish I agree that making this more general makes sense. If I understand your suggestion correctly, the "translation_agent_model" class you propose would also potentially give us more flexibility in controlling additional functionality, e.g. the way the reflection is done (which now uses just a call to get_completion()). Or it could be helpful if we wanted to add support for a glossary as described here.

@Omaraldarwish
Copy link

Omaraldarwish commented Jul 2, 2024

@methanet yes!! I'm actually working on branch that pulls all parameters out so users can explicitly control them / experiment with the them -that's turning out to be quite the rebase from what is in main right now! Also not clear whether functional or object-oriented would be better. Could go both ways.

On the flip side, If we want a quick simple "bring your own model" refactor, it seems we only need the user to pass a class that implements get_completion and num_tokens_in_string. Check out the modified example script and source code here. This frees up model, temperature, and system prompt to the user -- p.s. still need to update docs and test suite!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants