-
Notifications
You must be signed in to change notification settings - Fork 312
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): safe require tiktoken
for webpack bundlers
#4433
Conversation
Overall package sizeSelf size: 6.71 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
BenchmarksBenchmark execution time: 2024-06-25 19:04:47 Comparing candidate commit 5099972 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 260 metrics, 6 unstable metrics. |
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.
Sam and I spoke OOB. Will need a test as the encodingForModel
typo alluded.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4433 +/- ##
===========================================
- Coverage 92.64% 65.42% -27.23%
===========================================
Files 116 95 -21
Lines 4173 2756 -1417
Branches 33 33
===========================================
- Hits 3866 1803 -2063
- Misses 307 953 +646 ☔ View full report in Codecov by Sentry. |
packages/datadog-plugin-openai/test/streamed-responses/completions.simple.txt
Show resolved
Hide resolved
* fix * change typo * add tiktoken as dev dependency for testing * change tests to check tiktoken usage * test in-house estimator separately * add tiktoken license to third party
* fix * change typo * add tiktoken as dev dependency for testing * change tests to check tiktoken usage * test in-house estimator separately * add tiktoken license to third party
* fix * change typo * add tiktoken as dev dependency for testing * change tests to check tiktoken usage * test in-house estimator separately * add tiktoken license to third party
* fix * change typo * add tiktoken as dev dependency for testing * change tests to check tiktoken usage * test in-house estimator separately * add tiktoken license to third party
What does this PR do?
Add-on to #4366, where we added support for leveraging
tiktoken
if it is installed to capture token metrics for theopenai
integration. However, this does not play nicely withwebpack
, which popular framework Next.js uses under the hood. The workaround involves settingresolve.fallback.tiktoken = false
inwebpack.config.js
ornext.config.js
. However, to make using the tracer as seamless as possible, this fix wraps the require in a function to bypass this behavior (by not directly requiring it).Additionally, cleans up some test logic and utilizes
tiktoken
as a dev dependency for regression purposes.Motivation
Fixes #4424
The following webpack error would come up when bundling:
Testing
Unit
Added
tiktoken
as a dev dependency for OpenAI tests. All tests now run against this, and any changes to that library or how we use it (ie typos) are reflected in failing tests. To test the in-house estimations, I moved theestimateTokens
function into its own file, and added a small set of regression tests to assert basic behavior. This can be cleaned up more in a refactor of the OpenAI integration & tests in a future PR.Local
Ran webpack on a test script importing
dd-trace
, before and after this fix. I was getting the error before, and not after. I was wondering why our Next.js tests weren't picking up on this, but I believe it's because of how we set up those tests, and how we use aserver.js
file, which I'm not sure gets compiled with webpack.