Skip to content
This repository has been archived by the owner on Jun 5, 2024. It is now read-only.

chore: Changed token_count to only use tokenCountCallback #272

Merged
merged 4 commits into from
Mar 11, 2024

Conversation

jsumners-nr
Copy link
Contributor

This PR resolves #271.

Copy link

codecov bot commented Mar 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.00%. Comparing base (d2af144) to head (e61bd2f).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #272      +/-   ##
==========================================
- Coverage   98.00%   98.00%   -0.01%     
==========================================
  Files          23       23              
  Lines        2004     2002       -2     
==========================================
- Hits         1964     1962       -2     
  Misses         40       40              
Flag Coverage Δ
unit-tests-16.x 46.90% <100.00%> (-0.06%) ⬇️
unit-tests-18.x 46.90% <100.00%> (-0.06%) ⬇️
unit-tests-20.x 46.90% <100.00%> (-0.06%) ⬇️
versioned-tests-16.x 97.75% <100.00%> (-0.01%) ⬇️
versioned-tests-18.x 97.75% <100.00%> (-0.01%) ⬇️
versioned-tests-20.x 97.75% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jsumners-nr jsumners-nr marked this pull request as ready for review March 8, 2024 15:05
@jsumners-nr jsumners-nr requested a review from bizob2828 March 8, 2024 15:05
lib/llm/bedrock-response.js Outdated Show resolved Hide resolved
tests/versioned/common.js Outdated Show resolved Hide resolved
@@ -189,7 +189,7 @@ tap.afterEach(async (t) => {
chatMsgs
})

t.llmSummary({ tx, modelId, chatSummary, tokenUsage: true, numMsgs: events.length - 1 })
t.llmSummary({ tx, modelId, chatSummary, numMsgs: events.length - 1 })
Copy link
Member

Choose a reason for hiding this comment

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

this should be restored

@@ -98,8 +98,6 @@ tap.test('non-conforming response is handled gracefully', async (t) => {
t.equal(res.finishReason, undefined)
t.same(res.headers, undefined)
t.equal(res.id, undefined)
t.equal(res.inputTokenCount, undefined)
Copy link
Member

Choose a reason for hiding this comment

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

this should be restored

Copy link
Member

Choose a reason for hiding this comment

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

i meant the entire file, sorry for the confusion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? What are we trying to test by doing so?

Copy link
Member

Choose a reason for hiding this comment

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

i understand this code is going away in #265. so it's probably fine to leave as is but the test coverage will be missing until we rip it all out.

@@ -117,8 +117,6 @@ tap.test('handles claude streams', async (t) => {
t.equal(br.finishReason, 'done')
t.equal(br.requestId, 'aws-req-1')
t.equal(br.statusCode, 200)
t.equal(br.inputTokenCount, 5)
Copy link
Member

Choose a reason for hiding this comment

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

this should be restored

Copy link
Member

Choose a reason for hiding this comment

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

i meant the entire file, sorry for the confusion

Copy link
Member

@bizob2828 bizob2828 left a comment

Choose a reason for hiding this comment

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

sorry for the back and forth. one last comment

@@ -98,8 +98,6 @@ tap.test('non-conforming response is handled gracefully', async (t) => {
t.equal(res.finishReason, undefined)
t.same(res.headers, undefined)
t.equal(res.id, undefined)
t.equal(res.inputTokenCount, undefined)
Copy link
Member

Choose a reason for hiding this comment

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

i meant the entire file, sorry for the confusion

@@ -117,8 +117,6 @@ tap.test('handles claude streams', async (t) => {
t.equal(br.finishReason, 'done')
t.equal(br.requestId, 'aws-req-1')
t.equal(br.statusCode, 200)
t.equal(br.inputTokenCount, 5)
Copy link
Member

Choose a reason for hiding this comment

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

i meant the entire file, sorry for the confusion

@jsumners-nr jsumners-nr requested a review from bizob2828 March 11, 2024 18:09
@jsumners-nr jsumners-nr merged commit a9dbf26 into newrelic:main Mar 11, 2024
11 checks passed
@jsumners-nr jsumners-nr deleted the issue-271 branch March 11, 2024 18:12
@github-actions github-actions bot mentioned this pull request Mar 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

bedrock: Only assign token_count from result of tokenCountCallback
2 participants