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(openai): support Python 3.12, 3.13 [backport 2.20] #12082

Merged
merged 1 commit into from
Jan 24, 2025

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jan 24, 2025

Backport 1760acf from #12014 to 2.20.

Resolves #11994.

We were previously not testing our OpenAI integration with Python versions >= 3.12 (at the time, OpenAI did not support those newer Python versions). We recently saw that our bytecode wrapping for OpenAI methods broke in Python 3.13, so this PR attempts to address that:

  • Switch out our bytecode wrapping with wrapt.wrap_function_wrapper() to make our patching compatible with newer Python versions.
  • Add Python 3.12 and 3.13 to tested versions for the OpenAI integration.
  • Implements unpatching and adds patching tests

Wrapping Changes

We previously were using direct bytecode wrapping in the OpenAI integration, which was not supported for Python 3.13, and instead switched to using wrapt.wrap_function_wrapper() to wrap OpenAI methods. This meant that we needed to change wrapping formats, including:

  • use function/attribute names (string) rather than references as inputs to the wrapping functions
  • replace nested functions with @with_traced_module() to pass the traced OpenAI module reference between traced methods
  • implement unpatching
  • add patching tests (note this is messy because we support both v0 and v1 openai versions which have separate method names)

Note: the issue in #11994 was only reproducible via the AsyncOpenAI.Moderations.create(), AsyncOpenAI.Embeddings.create() endpoints (chat and completions were not affected). However to avoid any risk introduced by the now unsupported bytecode wrapping, we are changing the entire OpenAI integration to use wrapt.wrap_function_wrapper() instead.

Testing Changes

Azure OpenAI tests

Azure OpenAI requests are changed in latest versions of OpenAI, which fail our cassette tests (recorded in previous versions of OpenAI). However on manual testing, our openai integration is unchanged in support for newer versions. With consideration for time/effort to produce new cassette test files which is time consuming, we are going to skip cassette tests for Azure endpoints with latest openai versions until we improve our testing framework entirely (move off cassette files). Here are the manual traces submitted with Azure OpenAI for reference:
Screenshot 2025-01-23 at 2 20 00 PM
Screenshot 2025-01-23 at 2 20 14 PM

Checklist

  • PR author has checked that all the criteria below are met
  • The PR description includes an overview of the change
  • The PR description articulates the motivation for the change
  • The change includes tests OR the PR description describes a testing strategy
  • The PR description notes risks associated with the change, if any
  • Newly-added code is easy to change
  • The change follows the library release note guidelines
  • The change includes or references documentation updates if necessary
  • Backport labels are set (if applicable)

Reviewer Checklist

  • Reviewer has checked that all the criteria below are met
  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Newly-added code is easy to change
  • Release note makes sense to a user of the library
  • If necessary, author has 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

Resolves #11994.

We were previously not testing our OpenAI integration with Python
versions >= 3.12 (at the time, OpenAI did not support those newer Python
versions). We recently saw that our bytecode wrapping for OpenAI methods
broke in Python 3.13, so this PR attempts to address that:
- Switch out our bytecode wrapping with `wrapt.wrap_function_wrapper()`
to make our patching compatible with newer Python versions.
- Add Python 3.12 and 3.13 to tested versions for the OpenAI
integration.
- Implements unpatching and adds patching tests

## Wrapping Changes
We previously were using direct bytecode wrapping in the OpenAI
integration, which was not supported for Python 3.13, and instead
switched to using `wrapt.wrap_function_wrapper()` to wrap OpenAI
methods. This meant that we needed to change wrapping formats,
including:
- use function/attribute names (string) rather than references as inputs
to the wrapping functions
- replace nested functions with `@with_traced_module()` to pass the
traced OpenAI module reference between traced methods
- implement unpatching
- add patching tests (note this is messy because we support both v0 and
v1 openai versions which have separate method names)

**Note**: the issue in #11994 was only reproducible via the
`AsyncOpenAI.Moderations.create(), AsyncOpenAI.Embeddings.create()`
endpoints (chat and completions were not affected). However to avoid any
risk introduced by the now unsupported bytecode wrapping, we are
changing the entire OpenAI integration to use
`wrapt.wrap_function_wrapper()` instead.

## Testing Changes

### Python 3.7 removal (Drop support for OpenAI v0)

Since ddtrace 3.0 will be removing support for Python 3.7, we are also
dropping support accordingly in this PR. This also coincides with
removing support for OpenAI v0 since v0 was last released more than a 16
months ago, and requires a large maintenance burden and pollutes the
codebase. Note that this PR will just drop testing support, but a future
PR will remove tracing support for v0 in the form of a refactor/cleanup
of the OpenAI integration.

### Azure OpenAI tests

Azure OpenAI requests are changed in latest versions of OpenAI, which
fail our cassette tests (recorded in previous versions of OpenAI).
However on manual testing, our openai integration is unchanged in
support for newer versions. With consideration for time/effort to
produce new cassette test files which is time consuming, we are going to
skip cassette tests for Azure endpoints with latest openai versions
until we improve our testing framework entirely (move off cassette
files). Here are the manual traces submitted with Azure OpenAI for
reference:
<img width="578" alt="Screenshot 2025-01-23 at 2 20 00 PM"
src="https://github.com/user-attachments/assets/19f34edc-4c20-4cfd-804f-3e7bce26f4df"
/>
<img width="545" alt="Screenshot 2025-01-23 at 2 20 14 PM"
src="https://github.com/user-attachments/assets/a58bc888-6bee-4d67-9fda-0661aed14423"
/>

## Checklist
- [x] PR author has checked that all the criteria below are met
- The PR description includes an overview of the change
- The PR description articulates the motivation for the change
- The change includes tests OR the PR description describes a testing
strategy
- The PR description notes risks associated with the change, if any
- Newly-added code is easy to change
- The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist
- [x] Reviewer has checked that all the criteria below are met
- Title is accurate
- All changes are related to the pull request's stated goal
- Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- Testing strategy adequately addresses listed risks
- Newly-added code is easy to change
- Release note makes sense to a user of the library
- If necessary, author has 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](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

(cherry picked from commit 1760acf)
@github-actions github-actions bot requested review from a team as code owners January 24, 2025 20:23
@datadog-dd-trace-py-rkomorn
Copy link

Datadog Report

Branch report: backport-12014-to-2.20
Commit report: b8bab65
Test service: dd-trace-py

✅ 0 Failed, 130 Passed, 1468 Skipped, 4m 15.97s Total duration (36m 29.55s time saved)

@pr-commenter
Copy link

pr-commenter bot commented Jan 24, 2025

Benchmarks

Benchmark execution time: 2025-01-24 21:03:36

Comparing candidate commit b8bab65 in PR branch backport-12014-to-2.20 with baseline commit e9aed4f in branch 2.20.

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

@Yun-Kim Yun-Kim closed this Jan 24, 2025
@Yun-Kim Yun-Kim reopened this Jan 24, 2025
Copy link
Contributor Author

CODEOWNERS have been resolved as:

.riot/requirements/107d8f2.txt                                          @DataDog/apm-python
.riot/requirements/130158f.txt                                          @DataDog/apm-python
.riot/requirements/18de44f.txt                                          @DataDog/apm-python
.riot/requirements/1ad89c5.txt                                          @DataDog/apm-python
.riot/requirements/2634bf7.txt                                          @DataDog/apm-python
.riot/requirements/4a85f6d.txt                                          @DataDog/apm-python
.riot/requirements/4d27459.txt                                          @DataDog/apm-python
.riot/requirements/530c983.txt                                          @DataDog/apm-python
.riot/requirements/b5d5a35.txt                                          @DataDog/apm-python
releasenotes/notes/fix-openai-313-29ec43cbf2f35947.yaml                 @DataDog/apm-python
tests/contrib/openai/cassettes/v1/completion_stream_wrong_api_key.yaml  @DataDog/ml-observability
.riot/requirements/1825740.txt                                          @DataDog/apm-python
ddtrace/contrib/internal/openai/_endpoint_hooks.py                      @DataDog/ml-observability
ddtrace/contrib/internal/openai/patch.py                                @DataDog/ml-observability
ddtrace/contrib/openai/__init__.py                                      @DataDog/ml-observability
riotfile.py                                                             @DataDog/apm-python
tests/contrib/openai/test_openai_llmobs.py                              @DataDog/ml-observability
tests/contrib/openai/test_openai_patch.py                               @DataDog/ml-observability
tests/contrib/openai/test_openai_v0.py                                  @DataDog/ml-observability
tests/contrib/openai/test_openai_v1.py                                  @DataDog/ml-observability
.riot/requirements/1e6bd37.txt                                          @DataDog/apm-python
.riot/requirements/df60af6.txt                                          @DataDog/apm-python

@Yun-Kim Yun-Kim enabled auto-merge (squash) January 24, 2025 21:07
@Yun-Kim Yun-Kim merged commit fe4d74a into 2.20 Jan 24, 2025
633 checks passed
@Yun-Kim Yun-Kim deleted the backport-12014-to-2.20 branch January 24, 2025 21:33
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.

2 participants