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: add _wrap function and support multiple Moderation responses #3

Merged
merged 1 commit into from
Jul 6, 2023

Conversation

estib
Copy link
Collaborator

@estib estib commented Jul 5, 2023

What does this PR do?

Big Changes:

  1. Instead of instrumenting each kind of API call in its own separate _instrument_*() function, adds a common _wrap() function that instruments each API call type listed in the new TO_WRAP dict. The hope is that this will make the code easier to maintain moving forward, since it makes "what does instrumentation mean" common across all the api calls, and it makes "what is getting instrumented" effectively just a series of configurations that can be tweaked and added to as necessary.
  2. Changes the openai.Moderation.create() instrumentation to support multiple response values (and changes the underlying test expectations)

Small Changes:

  1. doesn't import all of wrapt, just the wrap_function_wrapper that we need.
  2. removes Tracer from opentelemetry.trace import, since we don't really need it?
  3. swaps math.inf for the equivalent float('inf') so that we can drop the math import altogether.

Motivation

  • Hopefully easier to review / maintain for future readers / contributors
  • Support more uses of Moderation.create()

Testing

Tests pass locally, and the spans i captured in console matched my expectations.

@estib estib changed the title fix: add _wrap function and return wrapped functions when suppressed fix: add _wrap function and support multiple Moderation responses Jul 5, 2023
Copy link
Owner

@cartermp cartermp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dig the changes. Let's goooooo

@cartermp cartermp merged commit e5da5da into main Jul 6, 2023
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