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

Use an enum for configurable_alternatives to make the generated json schema nicer #11350

Merged
merged 8 commits into from
Oct 4, 2023

Conversation

nfcampos
Copy link
Collaborator

@nfcampos nfcampos commented Oct 3, 2023

No description provided.

@nfcampos nfcampos requested a review from eyurtsev October 3, 2023 17:54
@vercel
Copy link

vercel bot commented Oct 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Oct 4, 2023 2:45pm

@dosubot dosubot bot added the 🤖:improvement Medium size change to existing code to handle new use-cases label Oct 3, 2023
@@ -136,6 +136,7 @@ class _Config:
arbitrary_types_allowed = True

include = include or []
include = [i for i in include if i != "configurable"]
Copy link
Collaborator

@eyurtsev eyurtsev Oct 3, 2023

Choose a reason for hiding this comment

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

could we make it possible to generate schema that does not include "configurable" schema?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

how is that useful?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

opt in to expose configuration params twice seems overkill no?

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • either we do that or that key is treated differently from other top level keys of the RunnableConfig (which is ifne)
  • Any situations where a server picks up an existing configurable runnable without knowing that it's configurable and not wanting to expose configurability by default?

Comment on lines 242 to 243
which_enum = enum.StrEnum( # type: ignore[misc]
self.which.name, list(self.alternatives.keys()) + ["default"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we worry about this note from the docs on StrEnum?
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's fine, we only check these values in this component

alt_keys = self.alternatives.keys()
which_keys = tuple(Literal[k] for k in alt_keys) + ( # type: ignore
Literal["default"],
which_enum = enum.StrEnum( # type: ignore[misc]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Our of curiosity, what is the error this is suppressing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pylance/mypy doesn't like a non-constant value being passed in as the name of the enum, but from checking the code this doesnt raise an issue

@@ -136,6 +136,7 @@ class _Config:
arbitrary_types_allowed = True

include = include or []
include = [i for i in include if i != "configurable"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • either we do that or that key is treated differently from other top level keys of the RunnableConfig (which is ifne)
  • Any situations where a server picks up an existing configurable runnable without knowing that it's configurable and not wanting to expose configurability by default?

@baskaryan baskaryan merged commit b0893c7 into master Oct 4, 2023
@baskaryan baskaryan deleted the nc/runnable-alts-enum branch October 4, 2023 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖:improvement Medium size change to existing code to handle new use-cases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants