Skip to content

Conversation

@Yun-Kim
Copy link
Contributor

@Yun-Kim Yun-Kim commented May 17, 2023

Fixes #5884.

This PR adds handling for non-string request input params for the Embeddings.create() method. This API method accepts strings, array of strings, as well as token arrays (list of ints) and arrays of token arrays (list of list of ints), but the previous implementation only accounted for strings and array of strings. Token arrays or arrays of token arrays being passed in as arguments would then cause errors as the traced request handler for the Embeddings endpoint did not account for non-string types.

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.
  • Documentation is included (in-code, generated user docs, public corp docs).
  • OPTIONAL: PR description includes explicit acknowledgement of the performance implications of the change as reported in the benchmarks PR comment.

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.

@Yun-Kim Yun-Kim requested review from a team as code owners May 17, 2023 19:20
@Yun-Kim Yun-Kim force-pushed the yunkim/fix-openai-embeddings-arg branch from 1a188d9 to f590523 Compare May 17, 2023 19:24
@pr-commenter
Copy link

pr-commenter bot commented May 17, 2023

Benchmarks

Comparing candidate commit a598843 in PR branch yunkim/fix-openai-embeddings-arg with baseline commit f5854aa in branch 1.x.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 94 cases.

emmettbutler
emmettbutler previously approved these changes May 18, 2023
@Yun-Kim Yun-Kim requested review from emmettbutler and mabdinur May 18, 2023 17:46
@Yun-Kim Yun-Kim requested a review from mabdinur May 18, 2023 20:03
@Yun-Kim Yun-Kim enabled auto-merge (squash) May 18, 2023 20:38
@Yun-Kim Yun-Kim merged commit 3adb270 into 1.x May 18, 2023
@Yun-Kim Yun-Kim deleted the yunkim/fix-openai-embeddings-arg branch May 18, 2023 20:40
Yun-Kim added a commit that referenced this pull request May 18, 2023
…point (#5890)

Fixes #5884.

This PR adds handling for non-string request input params for the
`Embeddings.create()` method. This API method accepts strings, array of
strings, as well as token arrays (list of ints) and arrays of token
arrays (list of list of ints), but the previous implementation only
accounted for strings and array of strings. Token arrays or arrays of
token arrays being passed in as arguments would then cause errors as the
traced request handler for the Embeddings endpoint did not account for
non-string types.

## 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/contributing.html#Release-Note-Guidelines)
are followed.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] OPTIONAL: PR description includes explicit acknowledgement of the
performance implications of the change as reported in the benchmarks PR
comment.

## 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.
Yun-Kim added a commit that referenced this pull request May 18, 2023
…point (#5890)

Fixes #5884.

This PR adds handling for non-string request input params for the
`Embeddings.create()` method. This API method accepts strings, array of
strings, as well as token arrays (list of ints) and arrays of token
arrays (list of list of ints), but the previous implementation only
accounted for strings and array of strings. Token arrays or arrays of
token arrays being passed in as arguments would then cause errors as the
traced request handler for the Embeddings endpoint did not account for
non-string types.

## 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/contributing.html#Release-Note-Guidelines)
are followed.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] OPTIONAL: PR description includes explicit acknowledgement of the
performance implications of the change as reported in the benchmarks PR
comment.

## 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.
mabdinur added a commit that referenced this pull request May 19, 2023
… to 1.14] (#5910)

Backports #5890 to 1.14.

Fixes #5884.

This PR adds handling for non-string request input params for the
`Embeddings.create()` method. This API method accepts strings, array of
strings, as well as token arrays (list of ints) and arrays of token
arrays (list of list of ints), but the previous implementation only
accounted for strings and array of strings. Token arrays or arrays of
token arrays being passed in as arguments would then cause errors as the
traced request handler for the Embeddings endpoint did not account for
non-string types.

## 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/contributing.html#Release-Note-Guidelines)
are followed.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] OPTIONAL: PR description includes explicit acknowledgement of the
performance implications of the change as reported in the benchmarks PR
comment.

## 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.

Co-authored-by: Munir Abdinur <munir.abdinur@datadoghq.com>
Yun-Kim added a commit that referenced this pull request May 19, 2023
… to 1.13] (#5909)

Backports #5890 to 1.13.

Fixes #5884.

This PR adds handling for non-string request input params for the
`Embeddings.create()` method. This API method accepts strings, array of
strings, as well as token arrays (list of ints) and arrays of token
arrays (list of list of ints), but the previous implementation only
accounted for strings and array of strings. Token arrays or arrays of
token arrays being passed in as arguments would then cause errors as the
traced request handler for the Embeddings endpoint did not account for
non-string types.

## 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/contributing.html#Release-Note-Guidelines)
are followed.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] OPTIONAL: PR description includes explicit acknowledgement of the
performance implications of the change as reported in the benchmarks PR
comment.

## 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.
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.

"trunc()" function (contrib/openai /patch.py) is expecting a string instead of a list

4 participants