Skip to content
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

config: failback on "nop" if provider is not available #1509

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

goneri
Copy link
Contributor

@goneri goneri commented Jan 23, 2025

Failback on the nop provider if the requested provider is not available. This way,
the service can start despite the misconfiguration.

Another approch is to "fail fast" and raise a critical exception with a clear message
to let the user fix the configuration.

Failback on the `nop` provider if the requested provider is not available. This way,
the service can start despite the misconfiguration.

Another approch is to "fail fast" and raise a critical exception with a clear message
to let the user fix the configuration.
@goneri goneri marked this pull request as draft January 23, 2025 21:33
@goneri
Copy link
Contributor Author

goneri commented Jan 23, 2025

Draft because there is no test yet and overall I want to discuss this with @manstis first.

@manstis
Copy link
Contributor

manstis commented Jan 23, 2025

Hmm. I'll look properly tomorrow but the fallback to NOP should have already been working. How could I replicate the problem? Simply exclude configuration for a pipeline?

@manstis
Copy link
Contributor

manstis commented Jan 23, 2025

If I remember correctly I (should be) creating the NOP's early when the configuration is first loaded. REGISTRY should then be fully populated so the later lookups can't fail.... or so I believe. I'll check tomorrow.

@manstis
Copy link
Contributor

manstis commented Jan 23, 2025

@goneri if you can find time to mention how to reproduce the issue I'd like to look before you arrive tomorrow. Thanks.

@goneri
Copy link
Contributor Author

goneri commented Jan 23, 2025

@goneri if you can find time to mention how to reproduce the issue I'd like to look before you arrive tomorrow. Thanks.

It happens if I see the provider of ChatBot to dummy.

@manstis
Copy link
Contributor

manstis commented Jan 24, 2025

@goneri OK.. so I looked and looked.. but could not find anything that was, or could have been, doing what I thought.

Your PR therefore seems fine to me - thanks for identifying the problem and providing a fix.

I did create a similar fix that ensures people won't need to worry about the safety of REGISTRY.

I'll leave it to you to decide which you prefer (but if you could fix this in your PR it'd be appreciated).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants