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
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Added logging of model name and base URL before LLM invocations to help diagnose incorrect base URL configuration issues. The implementation extracts these values from serialized LLM parameters using a new utility function that handles both kwargs (ChatOpenAI) and repr string parsing (other providers).
Key changes:
- New
extract_model_name_and_base_url()utility innemoguardrails/logging/utils.pythat extracts model name and base URL from serialized parameters - Logs model/URL before each LLM call in
on_llm_startandon_chat_model_startcallbacks - Comprehensive test coverage for the extraction logic including edge cases
- Handles multiple URL attribute names across different providers (
api_base,azure_endpoint,base_url, etc.)
Note: A previous comment highlighted that the logging logic is duplicated between on_llm_start and on_chat_model_start methods, which remains an opportunity for refactoring.
Confidence Score: 5/5
- This PR is safe to merge with minimal risk
- The changes are well-implemented with comprehensive test coverage. The new utility function uses defensive programming (checking for key existence, empty strings, type validation). The regex patterns are safe and handle both single and double quotes. No security concerns or logical errors found. The only existing issue (code duplication) was noted in a previous comment but doesn't affect functionality.
- No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| nemoguardrails/logging/utils.py | 5/5 | New utility function to extract model name and base URL from serialized LLM parameters using kwargs and repr string parsing. Well-tested with comprehensive test coverage. |
| nemoguardrails/logging/callbacks.py | 4/5 | Added logging of model name and base URL before LLM invocations in both on_llm_start and on_chat_model_start methods. Logic is duplicated in both methods (see existing comment). |
Sequence Diagram
sequenceDiagram
participant App as Application
participant Handler as LoggingCallbackHandler
participant Utils as extract_model_name_and_base_url
participant Logger as log.info
App->>Handler: on_llm_start(serialized, prompts, ...)
activate Handler
Handler->>Handler: Initialize LLMCallInfo
Handler->>Utils: extract_model_name_and_base_url(serialized)
activate Utils
alt kwargs available
Utils->>Utils: Extract from kwargs['model_name']
Utils->>Utils: Extract from kwargs['openai_api_base']
end
alt repr string available
Utils->>Utils: Parse repr with regex for model
Utils->>Utils: Parse repr with regex for URL
end
Utils-->>Handler: return (model_name, base_url)
deactivate Utils
alt base_url exists
Handler->>Logger: log.info(f"Invoking LLM: model={model_name}, url={base_url}")
else only model_name exists
Handler->>Logger: log.info(f"Invoking LLM: model={model_name}")
else neither exists
Handler->>Logger: log.info("Invoking LLM")
end
Handler->>Logger: log.info("Invocation Params :: ...")
Handler->>Logger: log.info("Prompt :: ...")
deactivate Handler
Note over App,Logger: Same flow occurs in on_chat_model_start
1 file reviewed, no comments
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