Wire up six dead config settings - #542
Open
amirfz wants to merge 1 commit into
Open
Conversation
Six settings in sherpa_ai/config/__init__.py were defined and
documented but never read anywhere:
- MODEL_PRICING_CONFIG_PATH: PricingManager.__init__ now falls back to
it when no explicit config_path is given.
- ENABLE_COST_TRACKING: UsageLogger.log_usage now gates cost
accumulation and threshold alerting behind it, so it can actually be
turned off (usage-file logging is unaffected).
- FILE_SIZE_LIMIT: enforced in QuestionWithFileHandler.download_file
right after the response comes back, alongside the existing
FILE_TOKEN_LIMIT check.
- DAILY_LIMIT_REACHED_MESSAGE: returned from UserUsageTracker.check_usage
when the daily limit is hit, instead of a hardcoded string.
- AWS_ACCESS_KEY / AWS_SECRET_KEY: deleted. backup.py's boto3.client("s3")
never read them (boto3 uses its own standard credential chain via
AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY, ~/.aws/credentials, or IAM
role), and the names weren't even boto3's standard ones, so they did
nothing under either interpretation.
Wiring DAILY_LIMIT_REACHED_MESSAGE surfaced a bigger issue: two
check_usage methods were defined on UserUsageTracker, and Python
silently used the second (later) one everywhere. That second
definition dropped the message/whitelist/time_left fields entirely,
contradicting the class's own documented example in database/__init__.py.
Removed the dead duplicate and kept the richer implementation, adjusting
its token-left calculation to match the "remaining before this request"
semantics the existing integration test already relied on (the two
definitions disagreed on this).
Each fix has a dedicated test, and each was verified by mutating the
fix back out and confirming the corresponding test fails, then
reverting. Full suite: 388 passed, 2 skipped.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
amirfz
force-pushed
the
fix/wire-dead-config-settings
branch
from
August 1, 2026 15:09
ea8fb0a to
23e2186
Compare
| """Initialize the pricing manager.""" | ||
| self.pricing_data = {} | ||
| self._load_pricing_config(config_path) | ||
| self._load_pricing_config(config_path or cfg.MODEL_PRICING_CONFIG_PATH) |
Collaborator
There was a problem hiding this comment.
Are we using this env var? MODEL_PRICING_CONFIG_PATH
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Fixes #517. Six settings in
sherpa_ai/config/__init__.pywere defined and documented but never read anywhere.MODEL_PRICING_CONFIG_PATH—PricingManager.__init__now falls back to it when no explicitconfig_pathis given.ENABLE_COST_TRACKING—UsageLogger.log_usagenow gates cost accumulation and threshold alerting behind it, so it can actually be turned off (usage-file logging is unaffected).FILE_SIZE_LIMIT— enforced inQuestionWithFileHandler.download_fileright after the response comes back, alongside the existingFILE_TOKEN_LIMITcheck.DAILY_LIMIT_REACHED_MESSAGE— returned fromUserUsageTracker.check_usagewhen the daily limit is hit, instead of a hardcoded string.AWS_ACCESS_KEY/AWS_SECRET_KEY— deleted per the issue's suggestion.backup.py'sboto3.client("s3")never read them; boto3 uses its own standard credential chain, and these names weren't even the standard ones boto3 looks for.A bigger bug found along the way
Wiring
DAILY_LIMIT_REACHED_MESSAGEsurfaced something worse:UserUsageTrackerhad twocheck_usagemethod definitions. Python silently uses the second (later) one everywhere — the first, richer implementation (withmessage, whitelist handling,time_left) was completely unreachable dead code, even though it matches the class's own documented example indatabase/__init__.py. The live (second) definition dropped all of that.Removed the dead duplicate and kept the richer implementation, but had to reconcile a real semantic disagreement between the two: they computed
token-leftdifferently (remaining before vs after the pending request). Kept the "before" semantics since that's what the existing integration test already asserted.Verification
Each of the 5 fixes has a dedicated test. Each was verified by mutating the fix back out and confirming the corresponding test fails, then reverting:
MODEL_PRICING_CONFIG_PATHfallback removed →test_config_path_falls_back_to_env_settingfailsENABLE_COST_TRACKINGgate removed →test_disabled_cost_tracking_suppresses_alertsfailsFILE_SIZE_LIMITcheck removed →test_download_file_rejects_oversized_filefailsDAILY_LIMIT_REACHED_MESSAGEreverted to hardcoded string →test_check_usage_limits_remaining_tokens's new assertion failsTest plan
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com