fix: firmware TUI credential manager compatibility - #4
Conversation
Summary of ChangesHello @b3nw, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the system's compatibility by integrating Firmware.ai and NanoGPT as new AI providers. It introduces robust quota tracking mechanisms tailored to each provider's unique API, including 5-hour rolling windows for Firmware.ai and subscription-based daily/monthly limits for NanoGPT. The changes also refine how the system identifies providers from credentials and displays quota information, ensuring accurate and flexible usage management across diverse APIs. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
…tibility When users add a Firmware.ai API key through the TUI credential manager, requests would fail because LiteLLM didn't recognize the firmware/ prefix. Changes: - Add firmware to SCRAPED_PROVIDERS with default API base URL - Add firmware to LITELLM_PROVIDERS for TUI categorization - Set FIRMWARE_API_BASE env var early in provider registration This ensures ProviderConfig.convert_for_litellm() properly routes firmware requests with the correct api_base override.
8954447 to
03c5dfd
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for the Firmware.ai provider, including quota tracking via a background job, and also adds a provider for NanoGPT with subscription-based usage tracking. The changes are well-structured, with new providers following a consistent plugin pattern using mixins for quota tracking. The PR also includes improvements to credential provider detection and display logic for quota statistics. My review found one potential issue regarding how NanoGPT usage is counted, which could impact credential rotation. Otherwise, the changes look solid and improve the library's provider support and maintainability.
I am having trouble creating individual review comments. Click here to see my feedback.
src/rotator_library/usage_manager.py (1078)
It appears nanogpt has been added to _REQUEST_COUNT_PROVIDERS. This set is intended for providers where failed requests consume quota, which means request_count is used for load balancing instead of success_count.
However, the docstring in src/rotator_library/providers/nanogpt_provider.py states that NanoGPT tracks 'usage units' which are defined as 'successful operations'. This implies that only successful requests consume quota.
If this is the case, including nanogpt here could lead to incorrect credential rotation, as keys would be penalized for transient failures that don't actually consume quota. Could you please verify if failed requests to NanoGPT consume usage units? If they do not, nanogpt should be removed from this set.
_REQUEST_COUNT_PROVIDERS = {"antigravity", "gemini_cli", "chutes"}
There was a problem hiding this comment.
Code Review
This pull request adds support for the Firmware.ai provider. The changes correctly add the provider to the configuration files. However, there are a couple of areas for improvement.
First, litellm_providers.py, which is an auto-generated file, has been edited manually. This goes against the process outlined in the file's header and can lead to inconsistencies. It should be updated using the provided script.
Second, a default api_base for Firmware is set by modifying the environment variables at import time. This approach has global side effects and makes the configuration logic less clear. A more maintainable solution would be to handle this default within the ProviderConfig class.
Overall, the changes are in the right direction but could be implemented more robustly to align with the project's existing patterns and for better long-term maintainability.
| "firmware": { | ||
| "display_name": 'Firmware.ai', | ||
| "route": 'firmware/', | ||
| "api_key_env_vars": ['FIRMWARE_API_KEY'], | ||
| "api_base_env_vars": ['FIRMWARE_API_BASE'], | ||
| "api_base_url": 'https://app.firmware.ai/api/v1', | ||
| "endpoints": ['/chat/completions', '/models'], | ||
| "features": ['streaming'], | ||
| "model_count": 0, | ||
| }, |
There was a problem hiding this comment.
This file appears to have been edited manually, but the header comment on lines 5-7 explicitly states "DO NOT EDIT MANUALLY" and provides instructions for updating it using a script. Manually editing generated files can lead to inconsistencies and cause issues with future updates.
For example, the provider count on line 15 has not been updated from 103 to 104.
Please regenerate this file using the tools/litellm_scraper/sync_to_library.py script to ensure all data is consistent and correctly formatted.
| # Set default API bases for known providers that need them | ||
| # This ensures ProviderConfig sees these values when it initializes | ||
| _provider_default_api_bases = { | ||
| "firmware": "https://app.firmware.ai/api/v1", | ||
| } | ||
| for provider, default_base in _provider_default_api_bases.items(): | ||
| env_key = f"{provider.upper()}_API_BASE" | ||
| if not os.environ.get(env_key): | ||
| os.environ[env_key] = default_base |
There was a problem hiding this comment.
While this change works, modifying os.environ at module import time introduces global side effects and makes configuration logic harder to follow. A developer reading ProviderConfig wouldn't know the environment is being altered elsewhere to provide a default.
A more maintainable approach is to handle this default within the ProviderConfig class, for example, by having get_api_base() fall back to the api_base_url from SCRAPED_PROVIDERS if an environment variable isn't set. This centralizes configuration logic and avoids mutating the global environment.
Address PR feedback: - Revert manual edit to auto-generated litellm_providers.py - Remove env var injection from providers/__init__.py - Add FIRST_PARTY_PROVIDER_DEFAULTS for providers not in LiteLLM - Update _load_api_bases() to load defaults from multiple sources Includes both firmware and nanogpt (alias for nano-gpt naming mismatch).
- Check FIRST_PARTY_PROVIDER_DEFAULTS in addition to KNOWN_PROVIDERS when determining if a provider is custom - Clarify nanogpt comment to explain it handles naming variant, not alias
- Add FIRST_PARTY_PROVIDER_DEFAULTS keys to KNOWN_PROVIDERS set so firmware and nanogpt route correctly through convert_for_litellm() - Add blacklist filtering for first-party providers (defensive) - Add nanogpt to LITELLM_PROVIDERS for UI consistency - Restore API base override logging at DEBUG level
- Update is_known_provider() docstring to reflect it includes first-party - Clarify FIRST_PARTY_PROVIDER_DEFAULTS comment about LITELLM_PROVIDERS
1 participant
Summary
Test plan
--add-credential)firmware/model-nameroute correctly