Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Track token usage of iris requests #165
Track token usage of iris requests #165
Changes from 10 commits
fc44738
26e3873
aa50faf
9905460
e241d45
4502e30
3b81a30
74b1239
6bcb002
4324180
c9e89be
4c92900
6bd4b33
c79837d
4d61c85
3253c46
9fe9e0a
13c5db1
dd504fc
043264a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Add error handling for missing keys in 'response' dictionary
When accessing
response["message"]
,response["prompt_eval_count"]
,response["eval_count"]
, andresponse["model"]
, there's a risk of aKeyError
if any of these keys are missing. It's safer to use theget
method with default values or implement error handling to manage potential missing data.Apply this diff to handle missing keys gracefully:
📝 Committable suggestion
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.
💡 Codebase verification
Calls to
convert_to_iris_message
inapp/llm/external/ollama.py
do not match the updated function signature. Please update these calls to include all required parameters:message
,num_input_tokens
,num_output_tokens
, andmodel
.app/llm/external/ollama.py
:convert_to_iris_message
is called with insufficient arguments.🔗 Analysis chain
Ensure all calls to
convert_to_iris_message
are updated with new parametersSince the function
convert_to_iris_message
now includes additional parametersusage: Optional[CompletionUsage]
andmodel: str
, please verify that all calls to this function across the codebase have been updated to match the new signature to prevent any potential errors.You can run the following script to identify any calls that may need to be updated:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 85
Script:
Length of output: 723
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.
Consider using 0 as the default value for token counts
The current implementation uses -1 as the default value for
num_input_tokens
andnum_output_tokens
. However, this might be confusing as it's not a valid token count. For consistency with other parts of the codebase (e.g., ollama) and improved clarity, consider using 0 as the default value.Suggested change:
This change would make the default values more intuitive and consistent with other parts of the codebase.
📝 Committable suggestion
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.
Wouldn't this overwrite existing any token counts when this wrapper is used a second time?
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.
The token information is saved in every pipeline as soon as the LLM call is done, so the token counts can be overwritten without any loss