-
Notifications
You must be signed in to change notification settings - Fork 3
Added cost calculation for Image models #1174
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
Conversation
|
/windsurf-review |
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.
Other comments (6)
-
src/services/commonServices/Google/gemini_image_utils.py (101-101)
The `response.model_dump()` call might fail if the response object doesn't have this method. Consider adding a try-except block or checking if the method exists before calling it:
"usage_metadata": getattr(response, 'model_dump', lambda: {})().get('usage_metadata', {}) | {"total_images_generated": (len(gcp_urls))} - src/services/commonServices/common.py (609-615) The image cost calculation code doesn't include error handling. If `token_calculator.calculate_image_usage()` or `calculate_image_cost()` fails (e.g., for unsupported models), it will cause an unhandled exception. Consider adding try/except blocks to gracefully handle potential calculation errors.
- src/services/commonServices/common.py (615-615) The code assigns `total_cost` to the response without validating it. Consider adding a fallback value or validation to ensure `parsed_data['tokens'].get('total_cost')` returns a valid number to avoid potential issues with downstream code that expects a numeric value.
- src/services/utils/ai_middleware_format.py (181-182) The 'revised_prompt' field was moved from individual image items to the parent data object. This creates an inconsistency with how it's structured in the else branch and may break clients expecting this field in both places.
-
src/services/commonServices/Google/gemini_image_utils.py (101-101)
The parentheses around `len(gcp_urls)` are unnecessary and can be removed for cleaner code:
"usage_metadata": response.model_dump().get('usage_metadata', {}) | {"total_images_generated": len(gcp_urls)} - src/services/utils/token_calculation.py (175-175) The docstring for `calculate_total_cost` states it "Automatically detects image models and uses appropriate calculation", but there's no actual detection logic in the method. Either implement this detection logic or update the docstring to reflect the actual behavior.
💡 To request another review, post a new comment with "/windsurf-review".
| "image_url" : urls, | ||
| "permanent_url" : urls |
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 'url' variable was removed on line 175, but it's still being referenced in the else branch on lines 198-199. This will cause a NameError when processing a single image response from Gemini.
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.
there is no issue like this. its just a comment
| if modality == 'TEXT': | ||
| usage['text_output_tokens'] = token_count | ||
| elif modality == 'IMAGE': | ||
| usage['image_output_tokens'] = token_count |
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.
Similar to the input tokens, the output token calculation for Gemini should accumulate values rather than overwrite them:
| if modality == 'TEXT': | |
| usage['text_output_tokens'] = token_count | |
| elif modality == 'IMAGE': | |
| usage['image_output_tokens'] = token_count | |
| if modality == 'TEXT': | |
| usage['text_output_tokens'] = usage.get('text_output_tokens', 0) + token_count | |
| elif modality == 'IMAGE': | |
| usage['image_output_tokens'] = usage.get('image_output_tokens', 0) + token_count |
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.
no, there will be no multiple entries with the same modality
|
explained review chnages do it ASAP |
added the cost calculation for the image models
there were different type of calculations for the openai , gemini, imagen