fix: Agent component not detecting provider fields for WatsonX#12022
fix: Agent component not detecting provider fields for WatsonX#12022dkaushik94 wants to merge 3 commits into
Conversation
…detect model provider settings.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (41.96%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## release-1.8.0 #12022 +/- ##
=================================================
- Coverage 37.05% 35.12% -1.94%
=================================================
Files 1588 1588
Lines 77969 77976 +7
Branches 11803 11805 +2
=================================================
- Hits 28893 27389 -1504
- Misses 47454 48951 +1497
- Partials 1622 1636 +14
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ogabrielluiz
left a comment
There was a problem hiding this comment.
PR Review Summary
Critical (2)
- Swapped variable keys in
validate_model_provider_keybreak validation for both OpenAI and WatsonX (see inline comments)
Important (2)
- Agent component only calls
apply_provider_variable_config_to_build_configfor WatsonX, while LanguageModelComponent calls it for all providers - Overly broad
except (ValueError, Exception)swallows all errors inget_all_variables_for_provider
Minor (1)
- Import inside loop body in
apply_provider_variable_config_to_build_config
The core approach (threading user_id, env-first then DB-override, moving field visibility outside field_name == "model") looks good. The variable key swap is the blocking issue.
| from langchain_openai import ChatOpenAI # type: ignore # noqa: PGH003 | ||
|
|
||
| api_key = variables.get("OPENAI_API_KEY") | ||
| api_key = variables.get("OPENAI_APIKEY") |
There was a problem hiding this comment.
Bug: Variable key is wrong. Should be OPENAI_API_KEY (with underscore), not OPENAI_APIKEY.
MODEL_PROVIDER_METADATA in model_metadata.py:78 defines the key as "OPENAI_API_KEY". The variables dict passed to this function is built from that metadata, so it will have key "OPENAI_API_KEY", but this line looks up "OPENAI_APIKEY", which will never match. This means OpenAI API key validation is silently skipped and invalid keys are accepted.
| api_key = variables.get("OPENAI_APIKEY") | |
| api_key = variables.get("OPENAI_API_KEY") |
| from langchain_ibm import ChatWatsonx | ||
|
|
||
| api_key = variables.get("WATSONX_APIKEY") | ||
| api_key = variables.get("WATSONX_API_KEY") |
There was a problem hiding this comment.
Bug: Variable key is wrong. Should be WATSONX_APIKEY, not WATSONX_API_KEY.
MODEL_PROVIDER_METADATA in model_metadata.py:182 defines the key as "WATSONX_APIKEY". Same issue as the OpenAI one above. The keys appear to have been accidentally swapped between the two providers. WatsonX validation is silently skipped.
| api_key = variables.get("WATSONX_API_KEY") | |
| api_key = variables.get("WATSONX_APIKEY") |
|
|
||
| # Show/hide watsonx fields | ||
| is_watsonx = provider == "IBM WatsonX" | ||
| if is_watsonx: |
There was a problem hiding this comment.
Inconsistency: apply_provider_variable_config_to_build_config is only called for WatsonX here, but LanguageModelComponent calls it for ALL providers.
In language_model.py:138-144, this is called unconditionally when provider is truthy. But here it is guarded by if is_watsonx:, meaning env/DB prefilling probably will not work for OpenAI, Anthropic, Google, or Ollama in the Agent component. Is this intentional?
| # If found in DB, it overrides environment variable | ||
| if value and str(value).strip(): | ||
| values[var_key] = str(value) | ||
| except (ValueError, Exception): # noqa: BLE001 |
There was a problem hiding this comment.
Overly broad exception catch. (ValueError, Exception) catches everything.
Exception is a superclass of ValueError, so the tuple is redundant. This catches all exceptions. Database connection failures, timeouts, and programming bugs all get logged at DEBUG level with the misleading message "Variable not found in database." Consider narrowing to the specific exception that get_variable raises for "not found", and logging infrastructure errors at WARNING.
|
|
||
| # Check if current value is just the hardcoded component default | ||
| # We want to allow the Global Variable to strictly override these specific defaults | ||
| from lfx.base.models.watsonx_constants import IBM_WATSONX_URLS |
There was a problem hiding this comment.
Minor: Import inside loop body.
from lfx.base.models.watsonx_constants import IBM_WATSONX_URLS runs on every iteration. Move to the top of the function for clarity, and to avoid a potential ImportError crashing all providers if the watsonx module has issues.
|
Closing this PR since this was tackled in a different PR from @edwinjosechittilappilly |
Refactored model provider underlying methods to work with WatsonX to detect model provider settings.
Wired in for the component to auto-detect globally configured WatsonX fields.
LE-449