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

fix: replace hardcoded model class with dynamic parameter #1440

Closed

Conversation

FWangZil
Copy link
Contributor

Relates to

issue #1439

Description

  • What is the problem?
    In the generateText function, the model type was hard-coded, preventing the function from dynamically switching models based on external parameters.
  • What does this PR do?
    1. Removes the hard-coded model class from generateText and generateMessageResponse.
    2. Use the parameter to specify the desired model type.

Risks

  • Risk Level: Low
    • The change is confined to the internal logic of the generateText and generateMessageResponse function.

Background

  • Previously, generateText and generateMessageResponse did not allow dynamic model switching based on incoming parameters, limiting flexibility and making it cumbersome to use different models in various scenarios.
  • By removing the hard-coded approach, developers can easily specify or configure the desired model without modifying core code.

What kind of change is this?

  • Bug fix (fixes a reported issue)
  • New feature (adds functionality)
  • Other (please describe)

Documentation changes needed?

  • Yes
  • No

Testing

Where should a reviewer start?

  • Look at how generateText and generateMessageResponse now handles the incoming model parameter, and verify that any calling code is appropriately updated to use this new parameter.

Detailed testing steps

  • Call generateText and generateMessageResponse with a specific model parameter, confirming that it correctly switches to the chosen model.

@monilpat monilpat changed the base branch from main to develop December 24, 2024 18:22
Copy link
Collaborator

@monilpat monilpat left a comment

Choose a reason for hiding this comment

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

LGTM - but please confirm that this works with a screenshot and if so remove hard coding of gpt-4o in generateObject generatedObjectedDEPRECATED etc thanks

@odilitime
Copy link
Collaborator

Just ran into something that could really use it

@FWangZil
Copy link
Contributor Author

I found that I should deal with the judgment and choice of TiktokenModel of different AI providers more. Let me temporarily close this PR, improve it, and reopen it later.

@FWangZil FWangZil closed this Dec 25, 2024
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