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

fix(langchain): handle secret str api keys [backport 2.1] #7480

Merged
merged 2 commits into from
Nov 6, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 3, 2023

Backport 6dc61f5 from #7430 to 2.1.

Currently the anthropic chain implementation in langchain uses a pydantic SecretStr as an api key this is causing errors in our pipeline when ddtrace tries to format the api key.

With this PR: langchain-ai/langchain#12542 the OpenAI implementation will also start using a SecretStr. I'm sure at that point there will be a few more people asking why things are broken.

I'm struggling setting up and running the tests, riot doesn't print anything. And I have no experience with the cassettes testing methods. Can someone help with this? I think if we add a test that uses the Anthropic LLM we will see the failure before. And this will fix it.

I've updated the type comment to the function, but the env doesn't know about Pydantic so I don't know if this is a valid thing to do.

Checklist

  • Change(s) are motivated and described in the PR description.
  • Testing strategy is described if automated tests are not included in the PR.
  • Risk is outlined (performance impact, potential for breakage, maintainability, etc).
  • Change is maintainable (easy to change, telemetry, documentation).
  • Library release note guidelines are followed. If no release note is required, add label changelog/no-changelog.
  • Documentation is included (in-code, generated user docs, public corp docs).
  • Backport labels are set (if applicable)

Reviewer Checklist

  • Title is accurate.
  • No unnecessary changes are introduced.
  • Description motivates each change.
  • Avoids breaking API changes unless absolutely necessary.
  • Testing strategy adequately addresses listed risk(s).
  • Change is maintainable (easy to change, telemetry, documentation).
  • Release note makes sense to a user of the library.
  • Reviewer has explicitly acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment.
  • Backport labels are set in a manner that is consistent with the release branch maintenance policy
  • If this PR touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

Currently the anthropic chain implementation in langchain uses a
pydantic SecretStr as an api key this is causing errors in our pipeline
when ddtrace tries to format the api key.

With this PR: langchain-ai/langchain#12542 the
OpenAI implementation will also start using a SecretStr. I'm sure at
that point there will be a few more people asking why things are broken.

I'm struggling setting up and running the tests, riot doesn't print
anything. And I have no experience with the cassettes testing methods.
Can someone help with this? I think if we add a test that uses the
Anthropic LLM we will see the failure before. And this will fix it.

I've updated the type comment to the function, but the env doesn't know
about Pydantic so I don't know if this is a valid thing to do.

## Checklist

- [X] Change(s) are motivated and described in the PR description.
- [x] Testing strategy is described if automated tests are not included
in the PR.
- [X] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [X] Change is maintainable (easy to change, telemetry, documentation).
- [X] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [X] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
- [x] If this PR touches code that signs or publishes builds or
packages, or handles credentials of any kind, I've requested a review
from `@DataDog/security-design-and-guidance`.
- [x] This PR doesn't touch any of that.

---------

Co-authored-by: Yun Kim <[email protected]>
Co-authored-by: Yun Kim <[email protected]>
(cherry picked from commit 6dc61f5)
@pr-commenter
Copy link

pr-commenter bot commented Nov 3, 2023

Benchmarks

Benchmark execution time: 2023-11-03 21:16:03

Comparing candidate commit a870b01 in PR branch backport-7430-to-2.1 with baseline commit 4e7c00a in branch 2.1.

Found 1 performance improvements and 2 performance regressions! Performance is the same for 87 metrics, 0 unstable metrics.

scenario:flasksimple-appsec-get

  • 🟥 max_rss_usage [+844.543KB; +1013.403KB] or [+2.503%; +3.004%]

scenario:flasksimple-appsec-post

  • 🟥 max_rss_usage [+795.457KB; +975.244KB] or [+2.356%; +2.888%]

scenario:span-add-metrics

  • 🟩 max_rss_usage [-14.751MB; -14.567MB] or [-25.799%; -25.478%]

@Yun-Kim Yun-Kim merged commit b0c2048 into 2.1 Nov 6, 2023
45 checks passed
@Yun-Kim Yun-Kim deleted the backport-7430-to-2.1 branch November 6, 2023 15:36
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.

3 participants