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

Enable/disable endpoints #281

Merged
merged 4 commits into from
Dec 5, 2023
Merged

Enable/disable endpoints #281

merged 4 commits into from
Dec 5, 2023

Conversation

eyurtsev
Copy link
Collaborator

@eyurtsev eyurtsev commented Dec 5, 2023

Add optionality to enable/disable endpoints

@cla-bot cla-bot bot added the cla-signed label Dec 5, 2023
@eyurtsev eyurtsev requested a review from jakerachleff December 5, 2023 02:22
*,
enabled_endpoints: Optional[Sequence[EndpointName]] = None,
disabled_endpoints: Optional[Sequence[EndpointName]] = None,
enable_feedback_endpoint: bool = False,
Copy link
Contributor

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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah compatibility :/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great tho!

Copy link
Collaborator Author

@eyurtsev eyurtsev Dec 5, 2023

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,
Copy link
Contributor

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

@eyurtsev eyurtsev requested a review from jakerachleff December 5, 2023 19:09
@eyurtsev eyurtsev merged commit 4a283dd into main Dec 5, 2023
10 checks passed
@eyurtsev eyurtsev deleted the eugene/endpoints_configs branch December 5, 2023 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants