-
Notifications
You must be signed in to change notification settings - Fork 229
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
Enable/disable endpoints #281
Conversation
*, | ||
enabled_endpoints: Optional[Sequence[EndpointName]] = None, | ||
disabled_endpoints: Optional[Sequence[EndpointName]] = None, | ||
enable_feedback_endpoint: bool = False, |
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 keep this arg?
endpoint_configuration = _EndpointConfiguration( | ||
enabled_endpoints=enabled_endpoints, | ||
disabled_endpoints=disabled_endpoints, | ||
enable_feedback_endpoint=enable_feedback_endpoint, |
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.
ah compatibility :/
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 looks great tho!
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.
(1) compatibility, (2) and for me this is an endpoint that should be default off
Regardless, if you're happy with this set up, i can update the tests and commit to this design. I can follow up with a PR to update handling of enable_feedback_point
per_req_config_modifier: Optional[PerRequestConfigModifier] = None, | ||
enable_feedback_endpoint: bool = False, |
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.
Should we get rid of this and see who complains? I feel like no one is probably using this outside of people we've actually pointed here, and it make the API gross to have it along with enabled/disabled endpoints...
I feel like we're really early on for backwards compat issues on langserve. Ideally we could do like minor version bumps or something and make small changes on compat
Add optionality to enable/disable endpoints