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

Standardized model init arg names #20085

Closed
1 task done
baskaryan opened this issue Apr 5, 2024 · 11 comments
Closed
1 task done

Standardized model init arg names #20085

baskaryan opened this issue Apr 5, 2024 · 11 comments
Labels
help wanted Good issue for contributors 🤖:improvement Medium size change to existing code to handle new use-cases

Comments

@baskaryan
Copy link
Collaborator

baskaryan commented Apr 5, 2024

Privileged issue

  • I am a LangChain maintainer, or was asked directly by a LangChain maintainer to create an issue here.

Issue Content

There's some discrepancies in the init args different models use to set the same params. It'd be a much nicer UX if common params could be set with a common set of init args

Suggesting that if a param in this list is present in a model integration, the below name should either be the actual attribute name or an init arg that can be used to set the attribute:

model: str  # model name
api_key: str  # api key
temperature: float  # temperature sampling
timeout: ...  # request timeout
max_tokens: int  # max tokens
stop: ...  # stop sequences
max_retries: int  # max num retries
base_url: str # base URL to send requests to

Importantly we should also use the above init args in the docs

@dosubot dosubot bot added the 🤖:improvement Medium size change to existing code to handle new use-cases label Apr 5, 2024
@baskaryan baskaryan added the 08 RFC Request for Comment. Solicit design input from other contributors label Apr 5, 2024
@liugddx
Copy link
Contributor

liugddx commented Apr 7, 2024

Do we need to refactor every llm?

@baskaryan
Copy link
Collaborator Author

Do we need to refactor every llm?

Ideally yes but we can do so incrementally, doesn't need to be all at once

@liugddx
Copy link
Contributor

liugddx commented Apr 8, 2024

Do we need to refactor every llm?

Ideally yes but we can do so incrementally, doesn't need to be all at once

I see.

@baskaryan baskaryan changed the title RFC: Unified model init args Unified model init args Apr 8, 2024
@baskaryan baskaryan changed the title Unified model init args Standardized model init args Apr 8, 2024
@baskaryan baskaryan added the help wanted Good issue for contributors label Apr 8, 2024
baskaryan added a commit that referenced this issue Apr 8, 2024
baskaryan added a commit that referenced this issue Apr 8, 2024
baskaryan added a commit that referenced this issue Apr 8, 2024
baskaryan added a commit that referenced this issue Apr 8, 2024
@sepiatone
Copy link
Contributor

sepiatone commented Apr 10, 2024

@baskaryan @liugddx

Do you think it's worth to set the above names as the actual attribute name (and have an alias for the current one)? eg., for OpenAI we have

param openai_api_key: Optional[SecretStr] = None (alias 'api_key')

which could be changed to

param api_key: Optional[SecretStr] = None (alias 'openai_api_key')

This will bring a common look and feel to the API references since sphinx(I think) arranges them in alphabetical order.

@sepiatone
Copy link
Contributor

sepiatone commented Apr 10, 2024

Also perhaps base_url (see #20170) -

Cohere

param base_url: Optional[str] = None

Anthropic

param anthropic_api_url: str = 'https://api.anthropic.com'

OpenAI

param openai_api_base: Optional[str] = None (alias 'base_url')

@liugddx
Copy link
Contributor

liugddx commented Apr 10, 2024

@baskaryan @liugddx

Do you think it's worth to set the above names as the actual attribute name (and have an alias for the current one)? eg., for OpenAI we have

param openai_api_key: Optional[SecretStr] = None (alias 'api_key')

which could be changed to

param api_key: Optional[SecretStr] = None (alias 'openai_api_key')

This will bring a common look and feel to the API references since sphinx(I think) arranges them in alphabetical order.

We need to be compatible, we try not to modify the original code.

@baskaryan
Copy link
Collaborator Author

baskaryan commented Apr 13, 2024

@baskaryan @liugddx

Do you think it's worth to set the above names as the actual attribute name (and have an alias for the current one)? eg., for OpenAI we have

param openai_api_key: Optional[SecretStr] = None (alias 'api_key')

which could be changed to

param api_key: Optional[SecretStr] = None (alias 'openai_api_key')

This will bring a common look and feel to the API references since sphinx(I think) arranges them in alphabetical order.

issue with this is it's a breaking change (unless we add some extra explicit properties) since the field name is the actual attribute by which the param is accessible. so if you switched eg openai then ChatOpenAI.openai_api_key would stop working

@sepiatone
Copy link
Contributor

issue with this is it's a breaking change (unless we add some extra explicit properties) since the field name is the actual attribute by which the param is accessible. so if you switched eg openai then ChatOpenAI.openai_api_key would stop working

yes, you're right, should've put in some more thought

ccurme pushed a commit that referenced this issue Jul 29, 2024
- **Description:** Standardize QianfanEmbeddingsEndpoint, include:
  - docstrings, the issue #21983 
  - model init arg names, the issue #20085
ccurme pushed a commit that referenced this issue Aug 3, 2024
- **Description:** Standardize MiniMaxEmbeddings
  - docs, the issue #24856 
  - model init arg names, the issue #20085
ccurme pushed a commit that referenced this issue Aug 6, 2024
- **Description:** Standardize Tongyi LLM,include:
  - docs, the issue #24803
  - model init arg names, the issue #20085
ccurme pushed a commit that referenced this issue Aug 7, 2024
- **Description:** Standardize QianfanLLMEndpoint LLM,include:
  - docs, the issue #24803 
  - model init arg names, the issue #20085
ccurme pushed a commit that referenced this issue Aug 13, 2024
- **Description:** Standardize SparkLLM, include:
  - docs, the issue #24803 
  - to support stream
  - update api url
  - model init arg names, the issue #20085
olgamurraft pushed a commit to olgamurraft/langchain that referenced this issue Aug 16, 2024
- **Description:** Standardize QianfanEmbeddingsEndpoint, include:
  - docstrings, the issue langchain-ai#21983 
  - model init arg names, the issue langchain-ai#20085
olgamurraft pushed a commit to olgamurraft/langchain that referenced this issue Aug 16, 2024
- **Description:** Standardize MiniMaxEmbeddings
  - docs, the issue langchain-ai#24856 
  - model init arg names, the issue langchain-ai#20085
olgamurraft pushed a commit to olgamurraft/langchain that referenced this issue Aug 16, 2024
- **Description:** Standardize Tongyi LLM,include:
  - docs, the issue langchain-ai#24803
  - model init arg names, the issue langchain-ai#20085
olgamurraft pushed a commit to olgamurraft/langchain that referenced this issue Aug 16, 2024
- **Description:** Standardize QianfanLLMEndpoint LLM,include:
  - docs, the issue langchain-ai#24803 
  - model init arg names, the issue langchain-ai#20085
olgamurraft pushed a commit to olgamurraft/langchain that referenced this issue Aug 16, 2024
- **Description:** Standardize SparkLLM, include:
  - docs, the issue langchain-ai#24803 
  - to support stream
  - update api url
  - model init arg names, the issue langchain-ai#20085
ccurme added a commit that referenced this issue Aug 23, 2024
updated stop and request_timeout so they aliased to stop_sequences, and
timeout respectively. Added test that both continue to set the same
underlying attributes.

Related to
[20085](#20085)

Co-authored-by: ccurme <chester.curme@gmail.com>
ccurme added a commit that referenced this issue Aug 29, 2024
Thank you for contributing to LangChain!

community:premai[patch]: standardize init args

- updated `temperature` with Pydantic Field, updated the unit test.
- updated `max_tokens` with Pydantic Field, updated the unit test.
- updated `max_retries` with Pydantic Field, updated the unit test.

Related to #20085

---------

Co-authored-by: Isaac Francisco <78627776+isahers1@users.noreply.github.com>
Co-authored-by: ccurme <chester.curme@gmail.com>
@dosubot dosubot bot added the stale Issue has not had recent activity or appears to be solved. Stale issues will be automatically closed label Sep 9, 2024
@dosubot dosubot bot closed this as not planned Won't fix, can't repro, duplicate, stale Sep 16, 2024
@dosubot dosubot bot removed the stale Issue has not had recent activity or appears to be solved. Stale issues will be automatically closed label Sep 16, 2024
@hinthornw hinthornw reopened this Sep 16, 2024
ccurme added a commit to langchain-ai/langchain-aws that referenced this issue Sep 20, 2024
per langchain-ai/langchain#20085

---------

Co-authored-by: Chester Curme <chester.curme@gmail.com>
Copy link

dosubot bot commented Dec 16, 2024

Hi, @baskaryan. I'm Dosu, and I'm helping the LangChain team manage their backlog. I'm marking this issue as stale.

Issue Summary:

  • The issue suggests standardizing initialization argument names across models for consistency.
  • You and @liugddx discussed the feasibility of incremental refactoring.
  • @sepiatone proposed using common attribute names with aliases for backward compatibility.
  • Concerns about breaking changes and future implementation plans were raised.
  • Discussion included aligning parameter names like stop_sequences with stop.

Next Steps:

  • Please confirm if this issue is still relevant to the latest version of LangChain. If so, you can keep the discussion open by commenting here.
  • If there is no further input, the issue will be automatically closed in 7 days.

Thank you for your understanding and contribution!

@dosubot dosubot bot added the stale Issue has not had recent activity or appears to be solved. Stale issues will be automatically closed label Dec 16, 2024
@dosubot dosubot bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 23, 2024
@dosubot dosubot bot removed the stale Issue has not had recent activity or appears to be solved. Stale issues will be automatically closed label Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Good issue for contributors 🤖:improvement Medium size change to existing code to handle new use-cases
Projects
None yet
Development

No branches or pull requests

4 participants