chore(logging): Log model name and base URL before invoking LLMs #1465
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Looks like the failing |
|
Thanks for the PR @JashG ! I agree this feature looks really useful for debugging! I have one suggestion: it might be better to move this logging logic to the
Implementation details:
that way, the log would appear right before the existing “Invocation Params” entry in on_llm_start() and on_chat_model_start(), keeping all the LLM call logs together. what do you think? |
Yes, we merged the fix to develop. You might want to do a rebase 👍🏻 |
JashG
force-pushed
the
jgulabrai/add-llm-invocation-logging
branch
from
54d2f96 to
617ac25
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Adds logging of model name and base URL before LLM invocations to aid in debugging incorrect URL configurations.
Key Changes:
- Created new utility function
extract_model_name_and_base_url()innemoguardrails/logging/utils.pythat extracts model information from serialized LLM parameters via kwargs (forChatOpenAI) or repr string parsing (for other providers) - Modified
LoggingCallbackHandlerto log model name and URL in bothon_llm_startandon_chat_model_startcallbacks - Added comprehensive test coverage (9 test cases) for various provider formats and edge cases
- Minor type annotation improvement to
_infer_model_name()inactions/llm/utils.py
Implementation Details:
The extraction logic handles multiple URL attribute patterns (api_base, api_host, azure_endpoint, base_url, endpoint, endpoint_url, openai_api_base) to support various LLM providers. The code prioritizes kwargs values over repr string values to ensure accurate extraction.
Confidence Score: 4/5
- This PR is safe to merge with minor code quality considerations
- The implementation is solid with comprehensive test coverage and addresses a legitimate debugging need. Score reduced by 1 point due to code duplication in the callback handlers (the same logging logic appears twice), but this doesn't affect functionality or introduce bugs.
- Pay attention to
nemoguardrails/logging/callbacks.py- contains duplicated logging code that could be refactored
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| nemoguardrails/logging/utils.py | 5/5 | New utility file for extracting model name and base URL from serialized LLM parameters with comprehensive test coverage |
| nemoguardrails/logging/callbacks.py | 4/5 | Added logging of model name and base URL before LLM invocation in both on_llm_start and on_chat_model_start callbacks |
| tests/test_callbacks.py | 5/5 | Added comprehensive unit tests for the extract_model_name_and_base_url utility function covering various providers and edge cases |
| nemoguardrails/actions/llm/utils.py | 5/5 | Minor type annotation update to _infer_model_name to accept Runnable in addition to BaseLanguageModel, plus unused logger addition |
Sequence Diagram
sequenceDiagram
participant Client as LLM Client
participant CB as LoggingCallbackHandler
participant Utils as extract_model_name_and_base_url
participant Logger as log.info
Note over Client,Logger: String Prompt Flow
Client->>CB: on_llm_start(serialized, prompts)
CB->>Utils: extract_model_name_and_base_url(serialized)
Utils->>Utils: Check kwargs for model_name, openai_api_base
Utils->>Utils: Parse repr string for model/url patterns
Utils-->>CB: return (model_name, base_url)
alt base_url exists
CB->>Logger: log.info("Invoking LLM: model={model}, url={url}")
else only model_name exists
CB->>Logger: log.info("Invoking LLM: model={model}")
else neither exists
CB->>Logger: log.info("Invoking LLM")
end
CB->>Logger: log.info("Invocation Params")
CB->>Logger: log.info("Prompt")
Note over Client,Logger: Chat Messages Flow
Client->>CB: on_chat_model_start(serialized, messages)
CB->>Utils: extract_model_name_and_base_url(serialized)
Utils->>Utils: Check kwargs for model_name, openai_api_base
Utils->>Utils: Parse repr string for model/url patterns
Utils-->>CB: return (model_name, base_url)
alt base_url exists
CB->>Logger: log.info("Invoking LLM: model={model}, url={url}")
else only model_name exists
CB->>Logger: log.info("Invoking LLM: model={model}")
else neither exists
CB->>Logger: log.info("Invoking LLM")
end
CB->>Logger: log.info("Invocation Params")
CB->>Logger: log.info("Prompt Messages")
4 files reviewed, 1 comment
| # Log model name and base URL | ||
| model_name, base_url = extract_model_name_and_base_url(serialized) | ||
| if base_url: | ||
| log.info(f"Invoking LLM: model={model_name}, url={base_url}") | ||
| elif model_name: | ||
| log.info(f"Invoking LLM: model={model_name}") | ||
| else: | ||
| log.info("Invoking LLM") |
There was a problem hiding this comment.
style: The exact same logging logic appears in both on_llm_start (lines 68-75) and on_chat_model_start (lines 118-125). Extract to a helper method to reduce duplication and improve maintainability.
|
@Pouyanpi thanks for that pointer! I agree, centralizing the logic in |
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR adds logging of model name and base URL before LLM invocations to help debug issues with incorrect base URLs being used for models. The implementation extracts these values from serialized LLM configuration using both structured kwargs and regex parsing of repr strings.
Key Changes:
- Created
extract_model_name_and_base_url()utility function in newnemoguardrails/logging/utils.pyfile - Integrated logging calls in both
on_llm_start()andon_chat_model_start()callback handlers - Added comprehensive test coverage including edge cases for various LLM providers
- Updated type hint for
_infer_model_name()to acceptUnion[BaseLanguageModel, Runnable]
Implementation Approach:
- Primary extraction from kwargs (for
ChatOpenAIand similar providers) - Fallback to regex parsing of repr strings (for other providers like
ChatNIM) - Supports multiple URL attribute names:
api_base,api_host,azure_endpoint,base_url,endpoint,endpoint_url,openai_api_base
Confidence Score: 4/5
- This PR is safe to merge with minor improvements recommended
- The implementation is solid with comprehensive test coverage and addresses a real debugging need. Score reduced by 1 point due to: (1) a minor typo in comments, and (2) duplicate logging logic in two callback methods that could be refactored into a helper method for better maintainability. The core functionality is sound and well-tested.
- nemoguardrails/logging/callbacks.py has duplicate logging code that could be refactored, but this is a minor maintainability concern
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| nemoguardrails/logging/utils.py | 5/5 | New utility function added to extract model name and base URL from serialized LLM config using kwargs and regex parsing |
| nemoguardrails/logging/callbacks.py | 4/5 | Added logging of model name and base URL in both on_llm_start and on_chat_model_start handlers with duplicate logic |
Sequence Diagram
sequenceDiagram
participant App as Application
participant CB as LoggingCallbackHandler
participant Util as extract_model_name_and_base_url
participant LLM as LLM Provider
participant Log as Logger
App->>CB: on_llm_start(serialized, prompts, ...)
CB->>Util: extract_model_name_and_base_url(serialized)
alt Has kwargs
Util->>Util: Extract from kwargs["model_name"]
Util->>Util: Extract from kwargs["openai_api_base"]
end
alt No kwargs or missing values
Util->>Util: Parse serialized["repr"] string
Util->>Util: Regex search for model/model_name
Util->>Util: Regex search for url attributes
end
Util-->>CB: return (model_name, base_url)
alt base_url exists
CB->>Log: log.info("Invoking LLM: model={model_name}, url={base_url}")
else only model_name exists
CB->>Log: log.info("Invoking LLM: model={model_name}")
else neither exists
CB->>Log: log.info("Invoking LLM")
end
CB->>Log: log.info("Invocation Params :: ...")
CB->>Log: log.info("Prompt :: ...")
CB->>LLM: Continue with LLM invocation
1 file reviewed, 1 comment
| if match: | ||
| model_name = match.group(1) | ||
|
|
||
| # Extract base URL. The propety name may vary between providers, so try common attribute patterns. |
There was a problem hiding this comment.
syntax: Typo: "propety" should be "property"
| # Extract base URL. The propety name may vary between providers, so try common attribute patterns. | |
| # Extract base URL. The property name may vary between providers, so try common attribute patterns. |
3 participants
Description
This PR logs the model name and base URL before an LLM is invoked.
Some context: From the NeMo Guardrails microservice, we've run into different errors that can be root caused to the incorrect base URL being used for a model. This can be difficult to root cause, since neither the URL nor model gets logged at any point in the call chain.
Related Issue(s)
Checklist