-
-
Notifications
You must be signed in to change notification settings - Fork 55
feat(quota): Add Firmware.ai quota tracking with 5-hour rolling window #92
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
|
I'm starting my review of the Firmware.ai quota tracking implementation. It's great to see the 5-hour rolling window being addressed! I'll dive into the details now. |
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.
Overall Assessment
The implementation of Firmware.ai quota tracking is solid and integrates well with the existing provider architecture. The handling of the 5-hour rolling window is a great addition, and the fallback logic for providers that only expose utilization percentages (rather than absolute request counts) is a significant improvement for the TUI display.
Architectural Feedback
- Virtual Model Tracking: Using
_quota(orfirmware/_quota) for credential-level tracking is a consistent and effective way to handle providers that don't expose per-model quotas. - Usage Manager Integration: The fallback logic in
UsageManagerto extract provider info from model names is a clever solution for handling raw API keys.
Key Suggestions
- Consistency in Model Naming: Standardize on either
_quotaorfirmware/_quotathroughoutFirmwareProviderto avoid potential mismatches in the usage manager. - Refined Error Logging: Consider providing slightly more detail in the error logs where it is safe to do so, to aid in troubleshooting API issues.
- Parallel Fetching: The use of a semaphore to limit concurrency in
run_background_jobis excellent.
Questions for the Author
None at this time. The plan and implementation are very clear.
This review was generated by an AI assistant.
| } | ||
| except Exception as e: | ||
| # Log exception type only - message may contain sensitive data | ||
| lib_logger.warning(f"Failed to fetch Firmware.ai quota: {type(e).__name__}") |
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.
While logging only the exception type is good for security (avoiding leaking API keys or sensitive data), it might make debugging harder. Since the URL doesn't contain the API key in this case, consider if logging a truncated or sanitized version of the error message would be helpful.
| try: | ||
| # Handle 'Z' suffix by replacing with UTC offset | ||
| if iso_string.endswith("Z"): | ||
| iso_string = iso_string.replace("Z", "+00:00") |
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.
In Python 3.11+, datetime.fromisoformat() supports the Z suffix directly. If the project targets Python 3.11 or newer, you can simplify this. Otherwise, this manual replacement is a safe fallback for older versions.
|
@mirrobot-agent re-review |
|
@b3nw, I'm starting my re-review of the Firmware.ai quota tracking implementation. I'll check the latest changes and see if my previous suggestions were addressed. |
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.
Overall Assessment
The standardization on firmware/_quota for virtual model tracking is a great improvement and addresses my previous feedback perfectly. This ensures consistent provider resolution in the UsageManager. The integration with the 5-hour rolling window and the TUI display logic remains solid.
Architectural Feedback
- Consistent Model Naming: Using the
provider/modelpattern for virtual models likefirmware/_quotais an excellent practice that simplifies provider identification across the library. - TUI Display Fallback: The logic to use
avg_remaining_pctwhen absolute request counts are unavailable is a clean solution for providers like Firmware.
Key Suggestions
- Docstring Consistency: A minor nit regarding the docstring in
FirmwareProviderto match the updated model name.
Questions for the Author
None. The implementation is clear and well-structured.
This review was generated by an AI assistant.
|
@b3nw Resolve merge conflicts here so it can be merged |
bf41d7a to
75873b3
Compare
|
/mirrobot-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.
Follow-up Review
This update successfully implements Firmware.ai quota tracking and integrates it into the wider library ecosystem. The implementation is robust and addresses previous feedback regarding model naming consistency and secure error logging.
Assessment of New Changes:
- Firmware Provider: The new provider and its associated mixin correctly handle the 5-hour rolling window quota system. The use of
firmware/_quotafor credential-level tracking is consistent and well-implemented. - Improved Display Logic: The updates in
RotatingClientto fallback toavg_remaining_pctand handle percentage-based displays are a significant improvement for the TUI, especially for providers like Firmware that don't expose absolute request limits. - Usage Manager: The new fallback logic to extract provider info from model names (e.g.,
firmware/model) is a clever and necessary addition for supporting providers with virtual models. - Security: The error handling in
fetch_quota_usagehas been refined to avoid logging sensitive data, which is an excellent practice.
Overall Status:
The changes are high-quality, idiomatic, and ready to be merged.
This review was generated by an AI assistant.
Mirrowel
left a comment
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.
You forgot the AI PLAN.MD file
75873b3 to
1005d67
Compare
Implement quota tracking for Firmware.ai provider using their /api/v1/quota endpoint. The provider tracks a 5-hour rolling window quota where `used` is already a 0-1 ratio from the API. - Add FirmwareQuotaTracker mixin with configurable api_base - Add FirmwareProvider with background job for periodic quota refresh - Parse ISO 8601 reset timestamps with proper Z suffix handling - Validate API response types and clamp remaining fraction to 0.0-1.0 - Support FIRMWARE_QUOTA_REFRESH_INTERVAL env var (default: 300s)
1005d67 to
6f456da
Compare
Summary
FirmwareProviderwith periodic quota refreshFirmwareQuotaTrackermixin for fetching quota usage from/api/v1/quotaendpointfirmware/_quotafor quota group aggregationmax_requestsis unavailable (Firmware doesn't expose this)How it works
Firmware.ai uses a 5-hour rolling window quota system:
GET /api/v1/quotareturns{ used: float, reset: string|null }usedis already a 0-1 ratio (e.g., 0.026 = 2.6% used)resetis an ISO 8601 timestamp when quota resets, or null when no active windowImportant
Adds Firmware.ai quota tracking with a 5-hour rolling window using a new provider class and mixin, updating usage management and client display.
FirmwareProviderclass infirmware_provider.pyfor handling Firmware.ai API with 5-hour rolling window quota tracking.FirmwareQuotaTrackermixin infirmware_quota_tracker.pyfor fetching and parsing quota usage.get_quota_stats()inclient.pyto display quota percentage whenmax_requestsis unavailable.FirmwareProvideruses a virtual model_quotafor credential-level tracking.fetch_quota_usage()inFirmwareQuotaTrackerfetches quota data and calculates remaining fraction.nullreset timestamps and parses ISO 8601 timestamps.UsageManagerinusage_manager.pyto support Firmware.ai's rolling window and handle null reset timestamps.FIRMWARE_QUOTA_REFRESH_INTERVALfor refresh interval configuration.This description was created by
for 9d5165b. You can customize this summary. It will automatically update as commits are pushed.