-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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: resolve Model Issues and add huggingface dependency #2339
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Hey @berrytern
Thanks for the PR. Please review the comments.
@@ -157,6 +148,8 @@ def build_model(self) -> LanguageModel: | |||
f"Provider {self.provider} is not supported. Supported providers are: {', '.join(provider_map.keys())}" | |||
) | |||
|
|||
os.environ["OPENAI_API_KEY"] = self.api_key |
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.
We shouldn't change the users environment.
replicate_api_key=api_keys["replicate_api_key"], | ||
cohere_api_key=api_keys["cohere_api_key"], | ||
openrouter_api_key=api_keys["openrouter_api_key"], | ||
openai_api_key=self.api_key, |
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.
Won't this pass the same variable to all? I assume only one api key must be passed, otherwise it will try to run the wrong one.
299eaa5
to
4365e3d
Compare
|
||
os.environ["OPENAI_API_KEY"] = self.api_key | ||
api_keys: dict[str, Optional[str]] = {provider_map[self.provider]: self.api_key} | ||
os.environ[self.provider.upper() + "_API_KEY"] = self.api_key |
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.
We shouldn't change the user's environment.
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.
Sorry about that. I fixed it in my last commit.
Hi! I'm autofix.ci, a bot that automatically fixes trivial issues such as code formatting in pull requests. I would like to apply some automated changes to this pull request, but it looks like I don't have the necessary permissions to do so. To get this pull request into a mergeable state, please do one of the following two things:
|
Cannot auto-update because of conflicts. |
Cannot auto-update because of conflicts. |
- Change Huggingface-hub version from 0.20.0 to 0.22.0; - Internal model_id resolver not working, create a field to model_id;
…#2339) * chore: adding default values to Azure OpenAI mandatory component * fix: huggingface model component: - Change Huggingface-hub version from 0.20.0 to 0.22.0; - Internal model_id resolver not working, create a field to model_id; * feat: add HuggingFace as extra dependency * chore: remove redundant atribution on children * fix: remove user environment variables from ChatLiteLLMModelComponent --------- Co-authored-by: joaoguilhermeS <[email protected]> (cherry picked from commit 805df82)
Fixes:
model_id
field (auto resolution was broken);Refact: