-
Notifications
You must be signed in to change notification settings - Fork 31
Integrate UiPath-LangChain-Client and cleanup old clients #496
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: main
Are you sure you want to change the base?
Conversation
44addbc to
163ce88
Compare
b9fc17d to
e64365a
Compare
61eec0a to
e3a6075
Compare
e6c3ebf to
304f333
Compare
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.
What is the purpose of this file? My concerns:
- when a new model appears we want to make it available in prod in the same day. This would require a new deployment.
- when a client uses Bring Your Own model, they can name their models however they want, like MyCustomModel. We should not do any validations based on model name. We should also remove all the "Invalid model name" exceptions from uipath_langchain_client: https://github.com/UiPath/uipath-llm-client-python/blob/287ca44d28ed09d6c1786508dacef9958c43d4fb/packages/uipath_langchain_client/src/uipath_langchain_client/factory.py#L106
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 agree, I don't think this file is necessary, but I left it untouched because as I understood from @andrei-rusu, there might be some people who use it, and I think he uses it in the gym. But I personally like more the factory method with the discovery endpoint because it makes the package available when a new model appears.
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.
And yes, I can refactor as well the factory from uipath_langchain_client
|
The discovery API returns the API Flavors that a model supports. Some clients with BYO do support only one API flavor, for example just bedrock converse or just bedrock invoke. I don't see where the API Flavor returned by the LLMGw on the discovery API is taking into account when instantiating the LLM client. I see that for bedrock api_config.api_flavor is always set to "invoke" (in uipath_langchain_client), which I think is wrong. |
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 think you should increase the major version of uipath-langchain-python as it is not backwards compatible. Some methods used in uipath-agents-python are removed.
This new client package has 2 main components:
Summary of the Changes for this package:
Before merging this PR I think it's important to make sure that all the relevant components have been moved to the new client, and also that the new changes aren't breaking stuff that's working
Still WIP, but maybe not necessary for this merge:
Some personal recommendations: