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

Shorter model name params #840

Merged

Conversation

bborn
Copy link
Contributor

@bborn bborn commented Oct 15, 2024

Fixes #820:

  • renames chat_completion_model_name to chat_model
  • renames completion_model_name to completion_model
  • renames embeddings_model_name to embedding_model

This is a breaking change, and I updated the changelog and bumped the version to reflect that.

@qarol
Copy link
Contributor

qarol commented Oct 15, 2024

What do you think to release new versions following Semantic Versioning https://semver.org? If the release introduces breaking change, then bumping major version should help to catch potential problems earlier.

@bborn
Copy link
Contributor Author

bborn commented Oct 15, 2024

@qarol you're probably right - a breaking change should be a MAJOR version change according to server. @andreibondarev I'll leave that decision to you though...

CHANGELOG.md Outdated
- Deprecate Langchain::LLM::GooglePalm
- Allow setting response_object: {} parameter when initializing supported Langchain::LLM::* classes
- Allow setting response_object: {} parameter when initializing supported Langchain::LLM::\* classes
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need the slash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird - I'm not sure why that's there. I think Prettier added it or something. Removing ...

Gemfile.lock Outdated Show resolved Hide resolved
chat_completion_model_name: "anthropic.claude-v2",
completion_model_name: "anthropic.claude-v2",
chat_model: "anthropic.claude-v2",
completion_model: "anthropic.claude-v2",
embeddings_model_name: "amazon.titan-embed-text-v1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm thinking maybe we rename this as well, to embedding_model?

@andreibondarev
Copy link
Collaborator

@bborn Quick thought -- maybe since the 3 methods are called:

def embed
def complete
def chat

we should call the defaults:

:embed_model
:complete_model
:chat_model

What do you think?

@bborn
Copy link
Contributor Author

bborn commented Oct 15, 2024

@andreibondarev I think that makes sense. Updated.

My only concern is that complete_model just sounds kind of ... weird to me. But it's consistent with the method names so probably ok.

@andreibondarev
Copy link
Collaborator

andreibondarev commented Oct 23, 2024

What do you think to release new versions following Semantic Versioning https://semver.org? If the release introduces breaking change, then bumping major version should help to catch potential problems earlier.

@qarol I love that! Given that we're pre-1.0.0, do you suggest that we keep incrementing minor versions, 0.18.0, 0.19.0, 0.20.0, etc.?

@andreibondarev andreibondarev added the ready Ready to be merged label Oct 23, 2024
@andreibondarev
Copy link
Collaborator

@bborn Great PR! Thank you!

@andreibondarev andreibondarev merged commit cf2bafd into patterns-ai-core:main Oct 23, 2024
5 checks passed
@qarol
Copy link
Contributor

qarol commented Oct 24, 2024

What do you think to release new versions following Semantic Versioning https://semver.org? If the release introduces breaking change, then bumping major version should help to catch potential problems earlier.

@qarol I love that! Given that we're pre-1.0.0, do you suggest that we keep incrementing minor versions, 0.18.0, 0.19.0, 0.20.0, etc.?

@andreibondarev question is, what things blocks from releasing 1.0.0 ;) For pre-1.0.0 it's acceptable to bump just minor version no matter if it's a breaking change or new feature. For bugfixes increasing patch number should be enough IMO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready Ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename the model name related parameter names to shorter versions
3 participants