refactor(providers): remove deprecated text-completion provider helpers#1924
Open
adityasingh2400 wants to merge 2 commits into
Open
Conversation
Closes NVIDIA-NeMo#1774. Drops the deprecated text-completion provider surface scheduled for removal in 0.23.0: register_llm_provider and get_llm_provider_names from nemoguardrails/llm/providers, the matching methods on LangChainFramework, the _llm_providers registry, _discover_langchain_community_llm_providers, _patch_acall_method_to, _get_text_completion_provider, _init_text_completion_model, _init_gpt35_turbo_instruct, and the mode parameter on init_llm_model and LangChainFramework.create_model. The CLI provider picker now lists only chat completion providers and the docs no longer reference BaseLLM custom registration. Tests that exercised the removed paths are updated, converted to chat equivalents, or removed. Signed-off-by: Aditya Singh <adisin650@gmail.com>
Contributor
Documentation preview |
Contributor
📝 WalkthroughWalkthroughThis PR removes deprecated text completion provider support and refactors LLM initialization to chat-model-only operations. It eliminates the ChangesText Completion Removal and Chat-Only Refactoring
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Suggested Labels
Suggested Reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/integrations/langchain/llm/models/test_langchain_init_scenarios.py (1)
246-253: 💤 Low valueRemove redundant direct assignment to
_chat_providers.Line 250 duplicates what
registry.register_chat(provider, None)already does on line 249. Theregister_chatmethod sets_chat_providers[name] = Nonewhenprovider_cls is None.♻️ Suggested fix
def test_none_provider_handled(self, registry): """Provider registered as None doesn't crash.""" provider = "_test_none" registry.register_chat(provider, None) - _chat_providers[provider] = None with pytest.raises((ModelInitializationError, TypeError, AttributeError)): init_langchain_model("test-model", provider, {})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integrations/langchain/llm/models/test_langchain_init_scenarios.py` around lines 246 - 253, Remove the redundant direct assignment to the module-global _chat_providers in the test_none_provider_handled function: keep the call to registry.register_chat(provider, None) (which already sets _chat_providers[name] = None) and delete the subsequent line "_chat_providers[provider] = None" so the test only uses registry.register_chat and avoids duplicating internal state setup.nemoguardrails/integrations/langchain/langchain_initializer.py (1)
21-21: 💤 Low valueRemove unused
Unionimport.After the refactor to chat-only types,
Unionis no longer used in this file. All return types are nowOptional[BaseChatModel]orBaseChatModel.♻️ Suggested fix
-from typing import Any, Callable, Dict, Optional, Union +from typing import Any, Callable, Dict, Optional🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nemoguardrails/integrations/langchain/langchain_initializer.py` at line 21, The import list in langchain_initializer.py still includes an unused symbol `Union`; remove `Union` from the typing import so the line reads only the used names (e.g., `Any, Callable, Dict, Optional`), and ensure any functions or return annotations (such as those returning `Optional[BaseChatModel]` or `BaseChatModel`) remain unchanged; update the `from typing import ...` statement to drop `Union` and run tests/linting to confirm no other references exist.tests/integrations/langchain/llm/models/test_langchain_special_cases.py (1)
86-89: 💤 Low valueMinor: Extra space in skip reason.
There's a double space in "Requires NVIDIA" on line 88.
✏️ Suggested fix
`@pytest.mark.skipif`( not has_nvidia_ai_endpoints(), - reason="Requires NVIDIA AI Endpoints package", + reason="Requires NVIDIA AI Endpoints package", )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integrations/langchain/llm/models/test_langchain_special_cases.py` around lines 86 - 89, The skip decorator's reason string contains a double space; update the pytest.mark.skipif(reason="Requires NVIDIA AI Endpoints package") to remove the extra space so it reads "Requires NVIDIA AI Endpoints package" (locate the decorator using pytest.mark.skipif and the has_nvidia_ai_endpoints() check).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@nemoguardrails/integrations/langchain/langchain_initializer.py`:
- Line 21: The import list in langchain_initializer.py still includes an unused
symbol `Union`; remove `Union` from the typing import so the line reads only the
used names (e.g., `Any, Callable, Dict, Optional`), and ensure any functions or
return annotations (such as those returning `Optional[BaseChatModel]` or
`BaseChatModel`) remain unchanged; update the `from typing import ...` statement
to drop `Union` and run tests/linting to confirm no other references exist.
In `@tests/integrations/langchain/llm/models/test_langchain_init_scenarios.py`:
- Around line 246-253: Remove the redundant direct assignment to the
module-global _chat_providers in the test_none_provider_handled function: keep
the call to registry.register_chat(provider, None) (which already sets
_chat_providers[name] = None) and delete the subsequent line
"_chat_providers[provider] = None" so the test only uses registry.register_chat
and avoids duplicating internal state setup.
In `@tests/integrations/langchain/llm/models/test_langchain_special_cases.py`:
- Around line 86-89: The skip decorator's reason string contains a double space;
update the pytest.mark.skipif(reason="Requires NVIDIA AI Endpoints package") to
remove the extra space so it reads "Requires NVIDIA AI Endpoints package"
(locate the decorator using pytest.mark.skipif and the has_nvidia_ai_endpoints()
check).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4f4b98ba-869c-4659-8c86-6d7b3bca093e
📒 Files selected for processing (42)
docs/configure-rails/custom-initialization/custom-llm-providers.mdexamples/configs/llm/hf_pipeline_dolly/config.pyexamples/configs/llm/hf_pipeline_falcon/config.pyexamples/configs/llm/hf_pipeline_llama2/config.pyexamples/configs/llm/hf_pipeline_mosaic/config.pyexamples/configs/llm/hf_pipeline_vicuna/config.pynemoguardrails/cli/__init__.pynemoguardrails/cli/providers.pynemoguardrails/evaluate/utils.pynemoguardrails/integrations/langchain/langchain_initializer.pynemoguardrails/integrations/langchain/llm_adapter.pynemoguardrails/integrations/langchain/providers/__init__.pynemoguardrails/integrations/langchain/providers/providers.pynemoguardrails/llm/models/initializer.pynemoguardrails/llm/providers/__init__.pynemoguardrails/rails/llm/llmrails.pytests/cli/test_cli_main.pytests/cli/test_llm_providers.pytests/integrations/langchain/llm/models/test_langchain_init_scenarios.pytests/integrations/langchain/llm/models/test_langchain_initialization_methods.pytests/integrations/langchain/llm/models/test_langchain_initializer.pytests/integrations/langchain/llm/models/test_langchain_special_cases.pytests/integrations/langchain/llm/providers/test_deprecated_providers.pytests/integrations/langchain/llm/providers/test_providers.pytests/integrations/langchain/llm/providers/test_trtllm_provider.pytests/integrations/langchain/llm/test_langchain_integration.pytests/integrations/langchain/llm/test_version_compatibility.pytests/integrations/langchain/test_configs/with_custom_chat_model/config.ymltests/integrations/langchain/test_configs/with_custom_chat_model/custom_chat_model.pytests/integrations/langchain/test_configs/with_custom_llm/config.cotests/integrations/langchain/test_configs/with_custom_llm/config.pytests/integrations/langchain/test_configs/with_custom_llm/config.ymltests/integrations/langchain/test_configs/with_custom_llm/custom_llm.pytests/integrations/langchain/test_custom_llm.pytests/integrations/langchain/test_langchain_llm_adapter.pytests/integrations/langchain/test_server_streaming.pytests/integrations/langchain/test_streaming.pytests/llm/frameworks/test_registry.pytests/test_imports.pytests/test_provider_selection.pytests/test_providers.pytests/test_supported_llm_providers.py
💤 Files with no reviewable changes (13)
- nemoguardrails/integrations/langchain/providers/init.py
- tests/integrations/langchain/test_configs/with_custom_llm/config.co
- tests/integrations/langchain/llm/providers/test_deprecated_providers.py
- tests/integrations/langchain/llm/providers/test_trtllm_provider.py
- tests/integrations/langchain/test_configs/with_custom_llm/config.yml
- tests/integrations/langchain/test_configs/with_custom_llm/custom_llm.py
- nemoguardrails/rails/llm/llmrails.py
- tests/test_supported_llm_providers.py
- tests/integrations/langchain/test_configs/with_custom_llm/config.py
- tests/test_providers.py
- nemoguardrails/evaluate/utils.py
- nemoguardrails/llm/providers/init.py
- tests/integrations/langchain/test_custom_llm.py
Contributor
Greptile SummaryThis PR removes the deprecated text-completion provider surface ahead of the 0.23.0 release, dropping
|
| Filename | Overview |
|---|---|
| nemoguardrails/integrations/langchain/langchain_initializer.py | Removes _init_text_completion_model, _init_gpt35_turbo_instruct, _SPECIAL_MODEL_INITIALIZERS, and the mode parameter from the entire initializer chain; simplifies ModelInitializer to three chat-only initializers. |
| nemoguardrails/integrations/langchain/providers/providers.py | Drops _llm_providers, _discover_langchain_community_llm_providers, _patch_acall_method_to, register_llm_provider, get_llm_provider_names, and _get_text_completion_provider; the trtllm import removed leaving the subtree as dead code. |
| nemoguardrails/integrations/langchain/llm_adapter.py | Removes register_llm_provider and get_llm_provider_names from LangChainFramework; create_model now silently pops mode from kwargs for backward compatibility with existing configs. |
| nemoguardrails/llm/providers/init.py | Fully removes the deprecated register_llm_provider and get_llm_provider_names public API; register_chat_provider and get_chat_provider_names remain as the sole public surface. |
| nemoguardrails/llm/models/initializer.py | Removes mode parameter from init_llm_model; no longer injects mode into model_kwargs before delegating to the framework. |
| nemoguardrails/rails/llm/llmrails.py | Removes mode='chat' and mode=llm_config.mode from both init_llm_model call sites; the rest of the initialization path is unchanged. |
| nemoguardrails/cli/providers.py | Drops select_provider_type, select_provider_with_type, and the two-step provider-type picker; find_providers now calls select_provider() directly against chat-only providers. |
| examples/configs/llm/hf_pipeline_dolly/config.py | Calls register_chat_provider with a BaseLLM subclass; all five hf_pipeline examples share this mismatch (covered in prior review thread). |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["init_llm_model(model_name, provider_name, kwargs)"] --> B["LangChainFramework.create_model()"]
B --> C["kwargs.pop('mode', None)\n(back-compat silent drop)"]
C --> D["init_langchain_model(model_name, provider_name, kwargs)"]
D --> E["#1 _handle_model_special_cases\n(nvidia_ai_endpoints / nim)"]
E -->|"returns model"| Z["LangChainLLMAdapter(raw_llm)"]
E -->|"returns None"| F["#2 _init_chat_completion_model\n(langchain init_chat_model)"]
F -->|"returns model"| Z
F -->|"returns None"| G["#3 _init_community_chat_models\n(community chat models)"]
G -->|"returns model"| Z
G -->|"returns None"| H["ModelInitializationError"]
Reviews (2): Last reviewed commit: "fix(providers): address CI lint and test..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| HFPipelineDolly = get_llm_instance_wrapper(llm_instance=get_dolly_v2_3b_llm(), llm_type="hf_pipeline_dolly") | ||
|
|
||
| register_llm_provider("hf_pipeline_dolly", HFPipelineDolly) | ||
| register_chat_provider("hf_pipeline_dolly", HFPipelineDolly) |
Contributor
There was a problem hiding this comment.
BaseLLM subclass registered as a chat provider
get_llm_instance_wrapper returns a WrapperLLM class that inherits from LangChain's LLM (a BaseLLM subclass), not from BaseChatModel. After this PR, all five hf_pipeline_* examples now call register_chat_provider with this class, so it lands in _chat_providers and is returned from _init_community_chat_models.
At runtime, LangChainLLMAdapter.generate_async calls llm.ainvoke(messages) on a BaseLLM instance; LangChain converts the chat message list to a flat string before dispatching to the text-completion endpoint. The adapter then receives a raw string response and falls back to content = str(response). The same mismatch applies to all five hf_pipeline_* configs (dolly, falcon, llama2, mosaic, vicuna).
These examples will silently produce incorrect output rather than raising a type error. They either need a proper BaseChatModel wrapper or should be removed alongside the text-completion surface.
Prompt To Fix With AI
This is a comment left during a code review.
Path: examples/configs/llm/hf_pipeline_dolly/config.py
Line: 65
Comment:
**`BaseLLM` subclass registered as a chat provider**
`get_llm_instance_wrapper` returns a `WrapperLLM` class that inherits from LangChain's `LLM` (a `BaseLLM` subclass), not from `BaseChatModel`. After this PR, all five `hf_pipeline_*` examples now call `register_chat_provider` with this class, so it lands in `_chat_providers` and is returned from `_init_community_chat_models`.
At runtime, `LangChainLLMAdapter.generate_async` calls `llm.ainvoke(messages)` on a `BaseLLM` instance; LangChain converts the chat message list to a flat string before dispatching to the text-completion endpoint. The adapter then receives a raw string response and falls back to `content = str(response)`. The same mismatch applies to all five `hf_pipeline_*` configs (`dolly`, `falcon`, `llama2`, `mosaic`, `vicuna`).
These examples will silently produce incorrect output rather than raising a type error. They either need a proper `BaseChatModel` wrapper or should be removed alongside the text-completion surface.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
86
to
87
| _chat_providers: Dict[str, Type[BaseChatModel]] | ||
|
|
||
| _llm_providers.update(_discover_langchain_community_llm_providers()) | ||
| _patch_acall_method_to(_llm_providers) | ||
| _chat_providers = _discover_langchain_community_chat_providers() |
Contributor
There was a problem hiding this comment.
trtllm module and stale server-layer reference
TRTLLM was the sole hard-coded entry in the now-removed _llm_providers = {"trt_llm": TRTLLM}. After this PR the entire providers/trtllm/ subtree (llm.py, client.py, __init__.py) is no longer imported, registered, or exercised by any non-deleted code. It is dead code.
Additionally, nemoguardrails/server/schemas/utils.py line 77 still lists "trt_llm": {} in the PROVIDERS dict. That dict is consulted when the server enumerates supported model providers, so it advertises trt_llm as usable while the LLM initialisation chain will raise RuntimeError: Could not find chat provider 'trt_llm' for any config that sets engine: trt_llm.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemoguardrails/integrations/langchain/providers/providers.py
Line: 86-87
Comment:
**Orphaned `trtllm` module and stale server-layer reference**
`TRTLLM` was the sole hard-coded entry in the now-removed `_llm_providers = {"trt_llm": TRTLLM}`. After this PR the entire `providers/trtllm/` subtree (`llm.py`, `client.py`, `__init__.py`) is no longer imported, registered, or exercised by any non-deleted code. It is dead code.
Additionally, `nemoguardrails/server/schemas/utils.py` line 77 still lists `"trt_llm": {}` in the `PROVIDERS` dict. That dict is consulted when the server enumerates supported model providers, so it advertises `trt_llm` as usable while the LLM initialisation chain will raise `RuntimeError: Could not find chat provider 'trt_llm'` for any config that sets `engine: trt_llm`.
How can I resolve this? If you propose a fix, please make it concise.…oval Drop the now-unused Union import in langchain_initializer, apply ruff format to the two files affected by the deprecation removal, and remove the stale mode assertion from the API key environment variable test now that init_llm_model no longer accepts a mode parameter. Signed-off-by: Aditya Singh <adisin650@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
1 participant
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.
Closes #1774.
Removes the text-completion provider surface that was deprecated for the 0.23.0 release. On the public side this drops register_llm_provider and get_llm_provider_names from nemoguardrails/llm/providers along with the matching methods on LangChainFramework. Internally it removes the _llm_providers registry, _discover_langchain_community_llm_providers, _patch_acall_method_to, _get_text_completion_provider, _init_text_completion_model, _init_gpt35_turbo_instruct, and the mode parameter that flowed through init_llm_model, LangChainFramework.create_model, and init_langchain_model. The CLI provider picker now shows only chat-completion providers, the warnings.catch_warnings suppression around the deprecated calls is gone, and the custom LLM provider documentation no longer covers BaseLLM registration. Existing tests for the deprecated paths are removed; tests for the surrounding live paths are kept or updated to the chat-only flow and pass against the new initializer chain.
The 0.22.0 release just shipped, so the surface change lands cleanly on develop for 0.23.0.
Summary by CodeRabbit
Breaking Changes
register_llm_providerremoved; useregister_chat_providerinsteadBaseChatModelinstead ofBaseLLMDocumentation
Examples
CLI