fix: allow deepseek and openrouter providers in ModelConfig validation#180
Conversation
saschabuehrle
left a comment
There was a problem hiding this comment.
Thanks for the focused fix. I validated the patch locally and the targeted tests and Ruff pass.\n\nI am requesting changes for release fit rather than test correctness: this PR expands the public provider-validation surface for a provider we do not want to add new release-facing code/test coverage for right now.\n\nPlease either split this into a narrower PR for the non-controversial provider validation fix, or hold the broader provider-alignment change for a later release where we can handle provider positioning intentionally.\n\nValidation I ran locally:\n- python -m pytest -q -o addopts='' tests/test_model_config_provider_validation.py tests/test_config.py tests/test_production_readiness.py -m 'not integration and not requires_api and not slow' -> 159 passed\n- python -m ruff check cascadeflow/schema/config.py tests/test_model_config_provider_validation.py -> passed
|
Thanks for the detailed review and for validating the patch locally.
After revisiting the PR scope and your feedback, I realized that the
provider validation changes were intentionally designed as the core purpose
of this PR. The validation updates and the related test coverage are
tightly coupled, since the PR was specifically meant to align the provider
validation behavior for DeepSeek and OpenRouter within ModelConfig.
If I remove those provider-related validation changes as requested, the
remaining modifications become extremely minimal and no longer provide
meaningful standalone value. Because of that, I agree that it would be
better not to expand the public provider-validation surface in this release
cycle.
I think the best approach would be to either:
close/hold this PR for now, or
revisit it in a future release where broader provider alignment and
positioning changes can be handled intentionally.
If you could suggest specific changes, improvements, or areas that would
better fit the current release scope, I would be happy to work on them
based on your feedback.
Thanks
Nishant
…On Sat, May 16, 2026 at 1:56 AM Sascha Buehrle ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Thanks for the focused fix. I validated the patch locally and the targeted
tests and Ruff pass.\n\nI am requesting changes for release fit rather than
test correctness: this PR expands the public provider-validation surface
for a provider we do not want to add new release-facing code/test coverage
for right now.\n\nPlease either split this into a narrower PR for the
non-controversial provider validation fix, or hold the broader
provider-alignment change for a later release where we can handle provider
positioning intentionally.\n\nValidation I ran locally:\n- python -m pytest
-q -o addopts='' tests/test_model_config_provider_validation.py
tests/test_config.py tests/test_production_readiness.py -m 'not integration
and not requires_api and not slow' -> 159 passed\n- python -m ruff check
cascadeflow/schema/config.py tests/test_model_config_provider_validation.py
-> passed
—
Reply to this email directly, view it on GitHub
<#180 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BBMXPYJ5IJIL2IU5FC6DYTL4254PXAVCNFSM6AAAAACXU45AXKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DGMBRGE3TINZZGY>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Description
Fix inconsistency in
ModelConfig.validate_provider()where supported providers (deepseek,openrouter) were rejected during validation.The allow-list in schema validation did not include these providers, even though they are supported in provider capabilities, registry, and used in examples. This caused valid user configurations to fail.
Type of Change
Testing
Test Results