fix(proxy): resolve router_settings.plugins dotted paths and load plugins from installed packages - #33644
Conversation
…gins from installed packages Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
|
Greptile SummaryThis PR fixes two gaps in the routing-plugin pipeline:
Confidence Score: 4/5The change is safe to merge; it resolves a real runtime crash and adds clear early-failure validation at config load time with no regressions to existing paths. The refactoring is clean and the logic is correct throughout. The get_instance_fn fallback is intentional and properly scoped, the validation in resolve_routing_plugins correctly checks for async run beyond what the @runtime_checkable Protocol alone guarantees, and tests cover the new code paths without real network calls. litellm/proxy/types_utils/utils.py — the fallback else branch now silently drops the local-file path from the error message; worth a second read to confirm the operator debugging experience is acceptable.
|
| Filename | Overview |
|---|---|
| litellm/proxy/types_utils/utils.py | Removes the hard ImportError when a local file is absent next to the config and instead falls through to importlib.import_module; local-file precedence is preserved. |
| litellm/proxy/proxy_server.py | Extracts resolve_routing_plugins from the former resolve_complexity_router_plugins and wires it into load_config for router_settings.plugins; validation and early-failure semantics are preserved correctly. |
| tests/test_litellm/proxy/proxy_server/test_proxy_config.py | Adds four unit tests for resolve_routing_plugins and two async integration tests for load_config; all use tmp_path/monkeypatch with no real network calls. |
| tests/test_litellm/proxy/types_utils/test_get_instance_fn_runtime_gate.py | Three new tests cover the installed-package fallback, local-file precedence, and the missing-everywhere ImportError; module names are unique enough to avoid sys.modules collisions in practice. |
Reviews (1): Last reviewed commit: "fix(proxy): resolve router_settings.plug..." | Re-trigger Greptile
| " return context\n" | ||
| "\n" | ||
| "instance = _P()\n" | ||
| ) | ||
| monkeypatch.syspath_prepend(str(pkg_dir)) | ||
| config_dir = tmp_path / "cfg" | ||
| config_dir.mkdir() | ||
|
|
||
| result = get_instance_fn( | ||
| value="my_installed_plugin.instance", | ||
| config_file_path=str(config_dir / "config.yaml"), | ||
| ) | ||
|
|
||
| assert type(result).__name__ == "_P" | ||
|
|
||
|
|
||
| def test_local_module_file_wins_over_installed_package(tmp_path, monkeypatch): | ||
| # A local module file next to the config must still take precedence over an |
There was a problem hiding this comment.
sys.modules pollution across tests
monkeypatch.syspath_prepend restores sys.path after the test but does not clear sys.modules. When get_instance_fn falls back to importlib.import_module("my_installed_plugin"), Python caches the module in sys.modules. If another test in the same process later calls importlib.import_module("my_installed_plugin") (e.g. after a different syspath_prepend), it will silently receive the stale cached instance from this test's tmp_path/site directory rather than a freshly imported one. The module names used here are unique enough to avoid real collisions, but a monkeypatch.delitem(sys.modules, "my_installed_plugin", raising=False) at the end of the test (or via a finally/fixture) would make the isolation explicit and protect against future name reuse.
| else: | ||
| # Dynamically import the module | ||
| module = importlib.import_module(module_name) |
There was a problem hiding this comment.
Fallback import error loses local-path context
When both the local file check fails (file absent) and importlib.import_module also fails (package not installed), the ImportError handler on line 57-60 re-raises with "Could not import {instance_name} from {module_name}". Before this change, the error explicitly named the expected local file path ("Could not find module file /config/dir/module.py"), making it immediately obvious where the proxy looked. After the change, operators debugging a typo in a plugin path get no hint about the expected local-file location. Consider logging the attempted local path at debug level before falling through, or including it in the re-raised message.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
8a4f380
into
litellm_internal_staging
Relevant issues
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
The reproduction config points
router_settings.pluginsat a dotted path whose module lives in an installed package (onPYTHONPATH, not next to the config), so it exercises both gaps at onceSame curl for both runs, hitting the real Anthropic API
Before the fix (base
4cfc987f56), the request fails because the dotted-path string is never resolved and the pipeline tries toawait "lang_plugin.language_plugin_instance".run(context){"error":{"message":"Invalid request format: 'str' object has no attribute 'run'","type":"invalid_request_error","param":null,"code":"400"}}After the fix (commit
c9aa000507), the plugin is resolved from the installed package at config load, runs in the routing pipeline, and the request returns a real Anthropic completionType
🐛 Bug Fix
Changes
Two gaps surfaced while validating the routing-plugin pipeline against a live proxy
First,
router_settings.pluginswas passed straight through toRouter(plugins=...)as raw config values, so dotted-path strings were never turned intoRoutingPlugininstances. The Router then treated each string as a plugin and blew up at request time onawait "some.path".run(context). The complexity-router path already resolved itspluginsviaresolve_complexity_router_plugins, so this factors that logic into a sharedresolve_routing_pluginshelper and calls it forrouter_settings.pluginstoo during config load, failing fast with a clear error if an entry does not implement theRoutingPlugininterfaceSecond,
get_instance_fn(the dotted-path resolver behind both plugin paths andlitellm_settings.callbacks) only ever looked for a local module file next to the config whenconfig_file_pathwas set, and raisedImportErrorwhen that file was absent. That made it impossible to reference a plugin shipped as an installed Python package. It now prefers the local file when one exists and otherwise falls back toimportlib.import_module, somodule.instanceresolves whether the module is a local file or an installed package. The existing remote-loading gate (s3://,gcs://) is untouchedTests cover the installed-package fallback and local-file precedence in
get_instance_fn, the newresolve_routing_pluginshelper, and an end-to-endload_configregression assertingrouter_settings.pluginsdotted paths land on the Router as live instances and that a bad entry raises at load timeFinal Attestation
Link to Devin session: https://app.devin.ai/sessions/605a563c77f94b5d94b792e89ce5ab7e
Requested by: @krrish-berri-2