-
Notifications
You must be signed in to change notification settings - Fork 17.6k
Improvements to ChatPerplexity
Integration
#30802
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
base: master
Are you sure you want to change the base?
Improvements to ChatPerplexity
Integration
#30802
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
enable_search_classifier
parameter and update model default to sonar-proenable_search_classifier
parameter | update model default to sonar-pro
| modify example notebook to reflect best practices
enable_search_classifier
parameter | update model default to sonar-pro
| modify example notebook to reflect best practices enable_search_classifier
parameter | update model default to sonar-pro
| modify example notebook to reflect best practices
enable_search_classifier
parameter | update model default to sonar-pro
| modify example notebook to reflect best practices ChatPerplexity
Integration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Are we still intending to add the tools to langchain-community? Should we remove those?
@ccurme Removed the tool integration. Thanks for pointing this out. |
@ccurme anything else I can do to help with this? |
@@ -168,6 +168,8 @@ class StructuredOutput(BaseModel): | |||
"""Whether to stream the results or not.""" | |||
max_tokens: Optional[int] = None | |||
"""Maximum number of tokens to generate.""" | |||
enable_search_classifier: bool = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I run this I get
TypeError: Completions.create() got an unexpected keyword argument 'enable_search_classifier'
Does this need to be passed in through extra_body
, as with other Perplexity-specific params not supported by the OpenAI SDK? (see examples in docs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct, it needs to be passed in the extra body. Forgot to push the commit. Fixing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed these comments in this commit. I removed the enable_search_classifier
param, it's better to add it to the Perplexity documentation directly and let users pass it themselves if needed.
@@ -147,9 +147,9 @@ class StructuredOutput(BaseModel): | |||
""" # noqa: E501 | |||
|
|||
client: Any = None #: :meta private: | |||
model: str = "llama-3.1-sonar-small-128k-online" | |||
model: str = "sonar-pro" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not ideal to change default models and temperatures, as it changes behavior (and costs?) out from under users without warning.
Ideally we'd actually remove these defaults and just rely on whatever defaults Perplexity uses server-side. I'd be supportive of doing this in a v0.2 release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the default model because the one that was there (llama-3.1-sonar-small-128k-online
) has been deprecated, we do not support it anymore. sonar-pro
is our best performing model for a regular web search so it will most likely result in higher quality outputs, but if cost is a concern then we can use our cheapest alternative which is sonar
. These are all of our currently supported models.
I changed the inference parameters because the ones that I added are our defaults. If we remove them altogether this would then use what we have server-side which is also ok (exactly the same thing). But specifying a model is required. Additional info here in our API ref.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's preferable to have the following:
model
has no default, users must pass it in on inittemperature
defaults to null and is only included in request payloads if it is explicitly specified.
That is, we mirror usage of the API.
I don't feel strongly about this if you'd prefer to maintain defaults in the LangChain integration. In either case, if we are changing the defaults we probably should bump the minor version from 0.1 to 0.2, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to get this in but there are still some issues:
- Code does not run. Please try running tests locally:
make test
will run unit testsPPLX_API_KEY=pplx-... make integration_test
will run integration tests (with real HTTP requests)
- We are (inadvertently?) undoing multiple PRs here (see comments)
- Run
make format
andmake lint
locally to run the auto-formatter and linter, which will catch some of the errors.
- Run
- Docs need to be updated to reflect most recent changes.
additional_kwargs = {"citations": response.citations} | ||
for attr in ["images", "related_questions"]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we un-doing #30805?
@@ -39,8 +50,8 @@ | |||
from typing_extensions import Self | |||
|
|||
_BM = TypeVar("_BM", bound=BaseModel) | |||
_DictOrPydanticClass = Union[dict[str, Any], type[_BM], type] | |||
_DictOrPydantic = Union[dict, _BM] | |||
_DictOrPydanticClass = Union[Dict[str, Any], Type[_BM], Type] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we un-doing #30781?
@@ -147,9 +147,9 @@ class StructuredOutput(BaseModel): | |||
""" # noqa: E501 | |||
|
|||
client: Any = None #: :meta private: | |||
model: str = "llama-3.1-sonar-small-128k-online" | |||
model: str = "sonar-pro" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's preferable to have the following:
model
has no default, users must pass it in on inittemperature
defaults to null and is only included in request payloads if it is explicitly specified.
That is, we mirror usage of the API.
I don't feel strongly about this if you'd prefer to maintain defaults in the LangChain integration. In either case, if we are changing the defaults we probably should bump the minor version from 0.1 to 0.2, though.
"""Get the default parameters for calling PerplexityChat API.""" | ||
return { | ||
"max_tokens": self.max_tokens, | ||
"stream": self.streaming, | ||
"temperature": self.temperature, | ||
"enable_search_classifier": self.enable_search_classifier, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is now broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Removed it.
" decide whether to execute a full web search or bypass it, depending on the query.\n", | ||
" This helps to reduce unnecessary searches while still delivering accurate responses.\n", | ||
" \"\"\"\n", | ||
" llm = ChatPerplexity(model=\"sonar\", enable_search_classifier=True)\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears out of date.
feat(chat): bump ChatPerplexity to v0.2, require model on init, conditional params - Remove default for `model` and add a pre-init validator that errors (with model‑cards URL) if it’s not provided - Make `temperature`, `max_tokens`, `stream`, and `timeout` only appear in the request payload when explicitly set - Bump class docstring and version reference to v0.2
chore(chat_models): restore `_DictOrPydantic` alias and fix citations handling in `_generate` - Re‑introduce the `_DictOrPydantic` type alias alongside `_DictOrPydanticClass` - In `_generate`, initialize `additional_kwargs` with `{"citations": response.citations}` and always append `images` and `related_questions`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still seeing tests failing on CI, undoing previous PRs and other unrelated changes.
|
||
See full list of supported init args and their descriptions in the params section. | ||
|
||
Instantiate: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why delete these? These are standard components of our API references.
TL;DR:
Add
enable_search_classifier
parameterUpdate model default to
sonar-pro
Modify example notebook to reflect best practices
Updates the Perplexity API wrapper (ChatPerplexity) by adding a new parameter,
enable_search_classifier
, with a default value ofTrue
. This enables the use of a classifier to determine if search is necessary for a given API call. Additionally, the default model name has been updated from the deprecated "llama-3.1-sonar-small-128k-online" to "sonar-pro", keeping the integration current with the Perplexity API. The change allows users to both modify this new flag and rely on sensible defaults while preserving the flexibility of passing any additional parameters viamodel_kwargs
.Updates example notebook with best practices around using Perplexity and correct links to documentation.
Dependencies:
No additional dependencies are introduced with this change.
Twitter handle:
(@LiounisJames, @PPLXDevs)
Additional steps performed:
• Ran make format, make lint, and make test to ensure that the changes adhere to the code quality and testing guidelines.
Thank you for reviewing this contribution!