Skip to content

Conversation

@nfcampos
Copy link
Contributor

@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

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
Contributor 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
Contributor 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
Contributor 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
Contributor 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

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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants