-
Notifications
You must be signed in to change notification settings - Fork 15.8k
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
Fix: redundant casting to SecretStr API keys #14446
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
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.
Appreciate it @rancomp ! I'll actually recommend picking this up again next week. We're in the middle of a big package organizing effort (context here) that may cause merge conflicts with this that will have to be resolved later next week.
Great change though! Just going to need a few more days to merge it in :)
Hey @efriis ! Thanks, I was actually not aware of that big move. I'll revisit this PR after the merge is complete. |
As you can see from the merge conflict list, it's in 😅 Feel free to ping when this PR is updated! |
Hey @efriis , can you check it out now? There's another thing I wanted to do which is clean up the tests, but I think it's better to separate it to another PR. |
is it possible to keep the name of the function as-is in master for this pr? two reasons:
also, in general trying to NOT make backwards incompatible changes to langchain-core, which this is |
hey. yeah definitely agree with you. |
@hwchase17 @efriis I have a small change I'm proposing within |
…x/api_key-masking
@efriis this is a cleanup / standardization. Do you think it's still relevant? |
Ah I think this actually bypasses an important part of type checking, where some classes may want to require that the passed in string is not None. If you want to fix the case where it wasn't actually storing a masked SecretStr separately though, that would be great! |
I think there's confusion here. This PR:
What I can also change is that the API-key type hint is changed to What I can do is allow for the API-keys to be passed as |
values
dict. They should be masked and cast back to string only during thePOST
request. I fixed 1 occurance of this.SecretStr
and invoking.get_secret_value()
immediately. This pattern is confusing, and in order to clear this out I added anextract_secret_str : SecretStr | Str | None -> str
method.secretstr, secret_str, secret_string
names. I replaced all of them withsecretstr
.