Integrate LiteLLM and Deprecate Langchain for LLM calls #139
Conversation
WalkthroughProject migrates from LangChain to LiteLLM: replaces LangChain dependencies and integrations with LiteLLM, adds lightweight in-repo chat utilities, updates app wiring for provider/model parsing, streaming, session-state chat history, PDF handling, and a reset utility; updates .gitignore and adds a migration summary doc. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant S as Streamlit app (`app.py`)
participant C as ChatHelper (`helpers/chat_helper.py`)
participant H as LLM Helper (`helpers/llm_helper.py`)
participant L as LiteLLM Provider
U->>S: Submit message / upload PDF
S->>C: Add `HumanMessage` to history
S->>H: validate provider/model → get_litellm_llm(...)
alt provider/model valid
S->>H: stream_litellm_completion(messages, max_tokens, api_key...)
H->>L: issue streaming request
loop stream tokens
L-->>H: token chunk (delta/content)
H-->>S: yield chunk
S->>C: append/aggregate `AIMessage` chunks
end
else invalid provider/model
H-->>S: validation error
S-->>U: show error and halt
end
S-->>U: render final AI response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
helpers/llm_helper.py (2)
39-45: Retry config bug:Retryhas nobackoff_jitter.This raises a TypeError at import. Remove it and enable
respect_retry_after_header.Apply:
retries = Retry( - total=5, - backoff_factor=0.25, - backoff_jitter=0.3, - status_forcelist=[502, 503, 504], - allowed_methods={'POST'}, + total=5, + backoff_factor=0.25, + status_forcelist=[502, 503, 504], + allowed_methods=frozenset({'POST'}), + respect_retry_after_header=True, )
100-116: Azure validation too lenient.
str(valid_url)is always truthy; invalid URLs pass. Validate scheme/host explicitly.- if provider == GlobalConfig.PROVIDER_AZURE_OPENAI: - valid_url = urllib3.util.parse_url(azure_endpoint_url) - all_status = all( - [azure_api_version, azure_deployment_name, str(valid_url)] - ) - return all_status + if provider == GlobalConfig.PROVIDER_AZURE_OPENAI: + parsed = urllib3.util.parse_url(azure_endpoint_url or "") + if not (parsed and parsed.scheme in ("https", "http") and parsed.host): + return False + return all([azure_api_version, azure_deployment_name])app.py (4)
101-121: Azure arg names/order are confusing here.Names suggest URL then deployment, but later you pass them swapped. Standardize to
azure_endpoint_url,azure_deployment_nameand use keyword args.-def are_all_inputs_valid( +def are_all_inputs_valid( user_prompt: str, provider: str, selected_model: str, user_key: str, - azure_deployment_url: str = '', - azure_endpoint_name: str = '', + azure_endpoint_url: str = '', + azure_deployment_name: str = '', azure_api_version: str = '', ) -> bool: @@ - if not llm_helper.is_valid_llm_provider_model( - provider, selected_model, user_key, - azure_endpoint_name, azure_deployment_url, azure_api_version - ): + if not llm_helper.is_valid_llm_provider_model( + provider=provider, + model=selected_model, + api_key=user_key, + azure_endpoint_url=azure_endpoint_url, + azure_deployment_name=azure_deployment_name, + azure_api_version=azure_api_version, + ):
136-149: Fix swapped Azure args at call site.They’re passed as deployment, endpoint, version; switch to keywords to avoid mistakes.
- if not are_all_inputs_valid( - prompt_text, provider, llm_name, user_key, - az_deployment, az_endpoint, api_ver - ): + if not are_all_inputs_valid( + user_prompt=prompt_text, + provider=provider, + selected_model=llm_name, + user_key=user_key, + azure_endpoint_url=az_endpoint, + azure_deployment_name=az_deployment, + azure_api_version=api_ver, + ):
386-389: Avoid logging full user prompts (PII risk).Log length/metadata only.
- logger.info( - 'User input: %s | #characters: %d | LLM: %s', - prompt_text, len(prompt_text), llm_name - ) + logger.info('Prompt length: %d | LLM: %s', len(prompt_text), llm_name)
416-425: Fix Azure argument order in legacy LLM helper call sites
- app.py:136-138 – llm_helper.is_valid_llm_provider_model(...) still passes azure_endpoint_name, azure_deployment_url; swap to azure_endpoint_url, azure_deployment_name.
- app.py:380-382 – are_all_inputs_valid(...) call uses az_deployment, az_endpoint; reorder or use named args to match the updated signature.
🧹 Nitpick comments (9)
requirements.txt (2)
11-11: Unpinned deps risk non‑reproducible installs.Pin or constrain:
- google-generativeai
- numpy
- huggingface-hub
Apply this diff to pin conservatively (adjust as needed):
-google-generativeai # ~=0.8.3 +google-generativeai~=0.8.3 -numpy +numpy~=1.26 -huggingface-hub #~=0.24.5 +huggingface-hub~=0.24.5Also applies to: 24-24, 33-33
32-33: httpx/anyio alignment with LiteLLM.LiteLLM uses httpx; ensure httpx/anyio pins remain compatible across provider backends. Consider a constraints.txt to centralize versions.
LITELLM_MIGRATION_SUMMARY.md (2)
96-103: Azure mapping likely inaccurate.For LiteLLM, Azure typically expects model as azure/{deployment_name}, not azure/{model}. Update docs to avoid misconfiguration.
123-126: “No breaking changes” claim is optimistic.You replaced LangChain chat components with custom classes. While internal, call‑sites may need small adjustments. Reword to “minimal breaking changes; app updated accordingly.”
helpers/llm_helper.py (4)
46-49: Unused requests Session/adapter.You build
http_sessionbut never use it (LiteLLM uses httpx). Remove to reduce confusion.-adapter = HTTPAdapter(max_retries=retries) -http_session = requests.Session() -http_session.mount('https://', adapter) -http_session.mount('http://', adapter) +# NOTE: LiteLLM uses httpx; requests Session not required and removed.
209-221: Stream parsing: add dict fallback.Some providers return dicts. Yield text robustly.
- for chunk in response: - if hasattr(chunk, 'choices') and chunk.choices: + for chunk in response: + if isinstance(chunk, str): + yield chunk + continue + if isinstance(chunk, dict) and "choices" in chunk and chunk["choices"]: + choice = chunk["choices"][0] + text = (choice.get("delta", {}) or {}).get("content") or (choice.get("message", {}) or {}).get("content") + if text: + yield text + continue + if hasattr(chunk, 'choices') and chunk.choices: choice = chunk.choices[0] if hasattr(choice, 'delta') and hasattr(choice.delta, 'content'): if choice.delta.content: yield choice.delta.content elif hasattr(choice, 'message') and hasattr(choice.message, 'content'): if choice.message.content: yield choice.message.content
222-224: Log stacktrace on failure.Use
logger.exceptionto preserve traceback.- except Exception as e: - logger.error(f"Error in LiteLLM completion: {e}") - raise + except Exception: + logger.exception("Error in LiteLLM completion") + raise
249-252: Surface missing dependency early.Raising instead of returning None prevents silent failures downstream; app already catches exceptions. Optional, but clearer.
- if litellm is None: - logger.error("LiteLLM is not installed") - return None + if litellm is None: + raise ImportError("LiteLLM is not installed. pip install litellm")app.py (1)
592-595: Preserve traceback on generation errors.Use
logger.exceptionfor easier debugging.- st.error(APP_TEXT['content_generation_error']) - logger.error('Caught a generic exception: %s', str(ex)) + st.error(APP_TEXT['content_generation_error']) + logger.exception('Caught a generic exception')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
LITELLM_MIGRATION_SUMMARY.md(1 hunks)app.py(2 hunks)helpers/llm_helper.py(3 hunks)requirements.txt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
helpers/llm_helper.py (1)
global_config.py (1)
GlobalConfig(16-198)
app.py (1)
helpers/llm_helper.py (1)
get_litellm_llm(227-286)
🪛 Ruff (0.13.1)
helpers/llm_helper.py
183-183: Avoid specifying long messages outside the exception class
(TRY003)
223-223: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (3)
helpers/llm_helper.py (1)
290-291: BC alias retained.Keeping
get_langchain_llmalias is a good migration aid.app.py (1)
19-60: Custom chat classes look good.Simple, compatible with Streamlit chat types. No issues spotted.
requirements.txt (1)
10-10: Pin LiteLLM minor version (avoid surprise breaks). Update requirements.txt (line 10) fromlitellm>=1.55.0to a reproducible constraint such aslitellm~=1.55.0(locks to 1.55.x) orlitellm>=1.55.0,<2.0(allows minor bumps but blocks v2). Latest stable is v1.76.1 (Aug 30, 2025); no major-version bump since 1.55.x but minor releases can add config/API changes—scan the changelog before upgrading.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
helpers/llm_helper.py (1)
123-129: Fix Azure endpoint validation (str(None) is truthy).
parse_url()may returnNone, andstr(valid_url)is always truthy, so invalid URLs pass. Validate the parsed object and required fields.Apply:
- if provider == GlobalConfig.PROVIDER_AZURE_OPENAI: - valid_url = urllib3.util.parse_url(azure_endpoint_url) - all_status = all( - [azure_api_version, azure_deployment_name, str(valid_url)] - ) - return all_status + if provider == GlobalConfig.PROVIDER_AZURE_OPENAI: + parsed = urllib3.util.parse_url(azure_endpoint_url or "") + if not (parsed and parsed.scheme in {"https"} and parsed.host): + return False + return bool(azure_api_version and azure_deployment_name)app.py (1)
447-456: Stop mutating os.environ in helpers/llm_helper — pass keys per-request.
helpers/llm_helper.py still assigns os.environ["OPENROUTER_API_KEY"] and AZURE_* (helpers/llm_helper.py:220–227); remove those assignments and refactor to configure provider clients with the supplied api_key/azure_endpoint/azure_api_version instead of mutating process-wide env.
🧹 Nitpick comments (7)
helpers/llm_helper.py (2)
30-31: Relax API key validation to avoid rejecting valid keys (dot and longer lengths).Keys like
hf_.../sk-or-v1-.../some provider tokens can include.and exceed 94 chars. Current regex is too strict.Apply:
-# 94 characters long, only containing alphanumeric characters, hyphens, and underscores -API_KEY_REGEX = re.compile(r'^[a-zA-Z0-9_-]{6,94}$') +# 6–256 printable token chars commonly used by providers +API_KEY_REGEX = re.compile(r'^[A-Za-z0-9._-]{6,256}$')And keep only the regex check (no extra length checks needed).
Also applies to: 120-121
39-49: Remove unused requests Retry/session (dead code).
http_sessionandadapterare never used; LiteLLM uses httpx internally.Apply:
-retries = Retry( - total=5, - backoff_factor=0.25, - backoff_jitter=0.3, - status_forcelist=[502, 503, 504], - allowed_methods={'POST'}, -) -adapter = HTTPAdapter(max_retries=retries) -http_session = requests.Session() -http_session.mount('https://', adapter) -http_session.mount('http://', adapter) +# NOTE: LiteLLM uses httpx internally; requests retry adapter not needed here.app.py (5)
101-109: Rename Azure args for clarity; keep ordering consistent.The parameter names in
are_all_inputs_validare swapped (URL vs name), which is confusing even though the call flips them back. Rename for correctness.Apply:
-def are_all_inputs_valid( +def are_all_inputs_valid( user_prompt: str, provider: str, selected_model: str, user_key: str, - azure_deployment_url: str = '', - azure_endpoint_name: str = '', + azure_endpoint_url: str = '', + azure_deployment_name: str = '', azure_api_version: str = '', ) -> bool: @@ - if not llm_helper.is_valid_llm_provider_model( - provider, selected_model, user_key, - azure_endpoint_name, azure_deployment_url, azure_api_version - ): + if not llm_helper.is_valid_llm_provider_model( + provider, selected_model, user_key, + azure_endpoint_url, azure_deployment_name, azure_api_version + ):And in the caller:
- if not are_all_inputs_valid( - prompt_text, provider, llm_name, user_key, - az_deployment, az_endpoint, api_ver - ): + if not are_all_inputs_valid( + prompt_text, provider, llm_name, user_key, + az_endpoint, az_deployment, api_ver + ):Also applies to: 411-414
345-347: Don’t render user messages as JSON code blocks.This formats plain text oddly. Render user messages with
write()and AI messages that are JSON withcode().Apply:
- for msg in history.messages: - st.chat_message(msg.type).code(msg.content, language='json') + for msg in history.messages: + if msg.type == "ai": + st.chat_message("ai").code(msg.content, language="json") + else: + st.chat_message("user").write(msg.content)
241-245: Avoid double error display on invalid provider.
handle_error()already callsst.error(). The secondst.error()duplicates output.Apply:
- handle_error(f"Invalid provider selected: {selected_provider}", True) - st.error(f"Invalid provider selected: {selected_provider}") + handle_error(f"Invalid provider selected: {selected_provider}", True) st.stop()
141-146: Sync UI hint with relaxed API key rule (length and charset).Update the message to match the new validation (6–256 chars; allow dot).
Apply:
- ' is provided if the selected LLM requires it. An API key should be 6-94 characters' - ' long, only containing alphanumeric characters, hyphens, and underscores.\n\n' + ' is provided if the selected LLM requires it. An API key should be 6–256 characters' + ' long, using typical token characters (letters, digits, dot, hyphen, underscore).\n\n'
512-521: Make “payment required” guidance provider‑agnostic.This path now covers multiple providers (not just HF). Suggest neutral wording with next steps.
Apply:
- 'The available inference quota has exhausted.' - ' Please use your own Hugging Face access token. Paste your token in' - ' the input field on the sidebar to the left.' - '\n\nDon\'t have a token? Get your free' - ' [HF access token](https://huggingface.co/settings/tokens) now' - ' and start creating your slide deck! For gated models, you may need to' - ' visit the model\'s page and accept the terms or service.' - '\n\nAlternatively, choose a different LLM and provider from the list.', + 'The model endpoint reported “payment required” or insufficient credits.' + ' Please supply a valid API key with sufficient quota on the sidebar,' + ' or choose a different LLM/provider. For gated models, accept the' + ' provider’s terms on the model page before retrying.',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app.py(4 hunks)helpers/llm_helper.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app.py (2)
global_config.py (1)
GlobalConfig(16-198)helpers/llm_helper.py (1)
get_litellm_llm(253-312)
helpers/llm_helper.py (1)
global_config.py (1)
GlobalConfig(16-198)
🪛 Ruff (0.13.1)
app.py
400-401: Possible SQL injection vector through string-based query construction
(S608)
helpers/llm_helper.py
195-195: Avoid specifying long messages outside the exception class
(TRY003)
202-202: Avoid specifying long messages outside the exception class
(TRY003)
249-249: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (3)
helpers/llm_helper.py (3)
133-150: Harden Azure model-name handling to prevent azure/azure double-prefix.Even though you special-case Azure later, this helper remains a foot‑gun if used elsewhere.
Apply:
def get_litellm_model_name(provider: str, model: str) -> str: @@ - prefix = provider_prefix_map.get(provider) - if prefix: - return f"{prefix}/{model}" + prefix = provider_prefix_map.get(provider) + if prefix: + if provider == GlobalConfig.PROVIDER_AZURE_OPENAI and model.startswith("azure/"): + return model + return f"{prefix}/{model}" return model
253-313: Approve — LiteLLM wrapper is a pragmatic bridge.Lightweight wrapper with stream() keeps app changes minimal and is fine given the migration. Repo-wide search found no runtime references to langchain or get_langchain_llm; only mentions are in LITELLM_MIGRATION_SUMMARY.md (lines 39–41, 89).
197-206: Avoid mutating process env; pass provider-specific params per call and set timeoutFile: helpers/llm_helper.py — lines 197–250
os.environ changes are process-global (Streamlit = multi‑user risk). LiteLLM supports per-call params for Azure/OpenRouter (api_key, api_base/azure_endpoint, api_version, base_url) and stream=True — add REQUEST_TIMEOUT to request params and use logger.exception for full stack traces.
Apply:
@@ - # Prepare the request parameters - request_params = { + # Prepare the request parameters + request_params = { "model": litellm_model, "messages": messages, "max_tokens": max_tokens, "temperature": GlobalConfig.LLM_MODEL_TEMPERATURE, "stream": True, + "timeout": REQUEST_TIMEOUT, } @@ - # Set API key and any provider-specific params - if provider != GlobalConfig.PROVIDER_OLLAMA: - # For OpenRouter, set environment variable as per documentation - if provider == GlobalConfig.PROVIDER_OPENROUTER: - os.environ["OPENROUTER_API_KEY"] = api_key - # Optional: Set base URL if different from default - # os.environ["OPENROUTER_API_BASE"] = "https://openrouter.ai/api/v1" - elif provider == GlobalConfig.PROVIDER_AZURE_OPENAI: - # For Azure OpenAI, set environment variables as per documentation - os.environ["AZURE_API_KEY"] = api_key - os.environ["AZURE_API_BASE"] = azure_endpoint_url - os.environ["AZURE_API_VERSION"] = azure_api_version - else: - # For other providers, pass API key as parameter - api_key_to_use = get_litellm_api_key(provider, api_key) - request_params["api_key"] = api_key_to_use + # Set API key and any provider-specific params (no global env mutation) + if provider != GlobalConfig.PROVIDER_OLLAMA: + if provider == GlobalConfig.PROVIDER_OPENROUTER: + request_params.update({ + "base_url": OPENROUTER_BASE_URL, + "api_key": api_key, + # Optional but recommended by OpenRouter: + "headers": { + "HTTP-Referer": "https://github.com/barun-saha/slide-deck-ai", + "X-Title": "SlideDeck AI", + }, + }) + elif provider == GlobalConfig.PROVIDER_AZURE_OPENAI: + request_params.update({ + "azure_endpoint": azure_endpoint_url, + "api_version": azure_api_version, + "api_key": api_key, + # "azure_deployment": azure_deployment_name, # optional since model="azure/<deployment>" + }) + else: + request_params["api_key"] = get_litellm_api_key(provider, api_key) @@ - except Exception as e: - logger.error(f"Error in LiteLLM completion: {e}") + except Exception: + logger.exception("Error in LiteLLM completion") raise
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
helpers/llm_helper.py (1)
124-129: Azure endpoint validation always truthy; enforce scheme and host.
str(parse_url(...))is non-empty even for empty URL (“None”). Validate scheme/host explicitly.- valid_url = urllib3.util.parse_url(azure_endpoint_url) - all_status = all( - [azure_api_version, azure_deployment_name, str(valid_url)] - ) - return all_status + parsed = urllib3.util.parse_url(azure_endpoint_url or "") + if not (parsed.scheme in ("http", "https") and parsed.host): + return False + return all([azure_api_version, azure_deployment_name])
🧹 Nitpick comments (5)
helpers/llm_helper.py (5)
39-50: Remove unused requests Retry/session setup.
http_sessionandadapterare never used; keep the file lean to avoid confusion.-retries = Retry( - total=5, - backoff_factor=0.25, - backoff_jitter=0.3, - status_forcelist=[502, 503, 504], - allowed_methods={'POST'}, -) -adapter = HTTPAdapter(max_retries=retries) -http_session = requests.Session() -http_session.mount('https://', adapter) -http_session.mount('http://', adapter) +# NOTE: Requests session removed as LiteLLM uses httpx under the hood.
112-121: API key check contradicts docstring and may reject valid keys.Doc says “at least five characters,” but regex enforces 6–94 and specific charset. Prefer a simple length check.
- if api_key and API_KEY_REGEX.match(api_key) is None: - return False + if len(api_key) < 5: + return False
235-247: Harden stream parsing for dict payloads.LiteLLM stream chunks can be dicts. Add a dict-path fallback to avoid dropping tokens.
for chunk in response: - if hasattr(chunk, 'choices') and chunk.choices: + if hasattr(chunk, 'choices') and chunk.choices: choice = chunk.choices[0] if hasattr(choice, 'delta') and hasattr(choice.delta, 'content'): if choice.delta.content: yield choice.delta.content elif hasattr(choice, 'message') and hasattr(choice.message, 'content'): if choice.message.content: yield choice.message.content + elif isinstance(chunk, dict): + choices = chunk.get("choices") or [] + if choices: + delta = (choices[0].get("delta") or {}) + content = delta.get("content") or (choices[0].get("message") or {}).get("content") + if content: + yield content
247-249: Log exceptions with traceback.Use
logger.exceptionto retain stack traces.- except Exception as e: - logger.error(f"Error in LiteLLM completion: {e}") + except Exception: + logger.exception("Error in LiteLLM completion") raise
318-326: Demo input uses[hf]...which currently fails validation.Either add
hftoVALID_PROVIDERSor change the example to a supported provider until then.Example alternative:
- '[hf]mistralai/Mistral-7B-Instruct-v0.2', + '[or]mistralai/mistral-7b-instruct',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
helpers/llm_helper.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
helpers/llm_helper.py (1)
global_config.py (1)
GlobalConfig(16-198)
🪛 Ruff (0.13.1)
helpers/llm_helper.py
195-195: Avoid specifying long messages outside the exception class
(TRY003)
202-202: Avoid specifying long messages outside the exception class
(TRY003)
248-248: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (4)
helpers/llm_helper.py (4)
133-150: Defensive fix: avoid double “azure/” prefix if used elsewhere.Even though Azure is special-cased in
stream_litellm_completion, harden this utility to prevent accidentalazure/azure/...names.prefix = provider_prefix_map.get(provider) if prefix: - return f"{prefix}/{model}" + if provider == GlobalConfig.PROVIDER_AZURE_OPENAI and model.startswith("azure/"): + return model + return f"{prefix}/{model}" return model
170-214: Add request timeout.
REQUEST_TIMEOUTis defined but unused. Pass it to LiteLLM for resilience.request_params = { "model": litellm_model, "messages": messages, "max_tokens": max_tokens, "temperature": GlobalConfig.LLM_MODEL_TEMPERATURE, "stream": True, + "timeout": REQUEST_TIMEOUT, }
278-301: Wrapper looks good; ensure message shape matches callers.
.stream()returns an iterator of strings; aligns with upstream usage. No action.Please confirm downstream expects a simple
Iterator[str]and not LangChainAIMessageChunk.
315-315: Backward-compat alias is helpful.Alias maintains existing imports. Good.
|
Thanks for the PR. I had a quick glance and will look at again. In the meantime, can you pls check that the following minimal style guidelines are adhered to:
|
There was a problem hiding this comment.
Thanks. I have left a few comments.
Also, PR #131 introduced some changes to app.py that have been merged recently. Could you pls check if you can pull those changes to your local copy and update the PR?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
helpers/chat_helper.py (1)
38-41: Add return type for messages property (minor typing hygiene).Helps IDEs and readers. Example change below.
Apply this diff:
- def messages(self): + def messages(self) -> list['ChatMessage']: return st.session_state[self.key]And add this import at the top of the file:
from typing import List # if you prefer List[ChatMessage] stylehelpers/llm_helper.py (3)
233-235: Use logging.exception to preserve stack trace.Improves debuggability when streaming fails.
Apply this diff:
- except Exception as e: - logger.error('Error in LiteLLM completion: %s', e) - raise + except Exception: + logger.exception('Error in LiteLLM completion') + raise
129-135: Stricter Azure endpoint validation (ensure scheme and host).Current check casts the URL object to str, which can be truthy for invalid inputs.
Apply this diff:
- if provider == GlobalConfig.PROVIDER_AZURE_OPENAI: - valid_url = urllib3.util.parse_url(azure_endpoint_url) - all_status = all( - [azure_api_version, azure_deployment_name, str(valid_url)] - ) - return all_status + if provider == GlobalConfig.PROVIDER_AZURE_OPENAI: + parsed = urllib3.util.parse_url(azure_endpoint_url) + return all([ + azure_api_version, + azure_deployment_name, + getattr(parsed, 'scheme', None) in ('http', 'https'), + bool(getattr(parsed, 'host', None)), + ])
15-16: Avoid mutating sys.path; use package-relative imports.sys.path manipulation is fragile. Prefer proper package layout and absolute/relative imports.
app.py (5)
181-188: Use lazy logger formatting instead of f-strings.Aligns with the requested logging style.
Apply this diff:
- logger.warning(f"Could not parse provider from selectbox option: {selected_option}") + logger.warning('Could not parse provider from selectbox option: %s', selected_option)
191-197: Use lazy logger formatting instead of f-strings.Aligns with the requested logging style.
Apply this diff:
- logger.warning(f"Provider regex did not match for: {llm_provider_to_use}, using: {selected_provider}") + logger.warning('Provider regex did not match for: %s, using: %s', llm_provider_to_use, selected_provider)
60-79: Fix confusing Azure param names (endpoint vs. deployment).Names are swapped; clarify to reduce future mistakes. Update signature and docstring accordingly.
Apply this diff:
-def are_all_inputs_valid( +def are_all_inputs_valid( user_prompt: str, provider: str, selected_model: str, user_key: str, - azure_deployment_url: str = '', - azure_endpoint_name: str = '', + azure_endpoint_url: str = '', + azure_deployment_name: str = '', azure_api_version: str = '', ) -> bool: @@ - :param azure_deployment_url: Azure OpenAI deployment URL. - :param azure_endpoint_name: Azure OpenAI model endpoint. + :param azure_endpoint_url: Azure OpenAI endpoint URL. + :param azure_deployment_name: Azure OpenAI deployment name. :param azure_api_version: Azure OpenAI API version.
95-101: Update internal validation call to use corrected Azure param order.Apply this diff:
- if not llm_helper.is_valid_llm_provider_model( - provider, selected_model, user_key, - azure_endpoint_name, azure_deployment_url, azure_api_version - ): + if not llm_helper.is_valid_llm_provider_model( + provider, selected_model, user_key, + azure_endpoint_url, azure_deployment_name, azure_api_version + ):
370-373: Swap args at callsite to match corrected Azure param names.Current order passes name before endpoint; reverse them.
Apply this diff:
- if not are_all_inputs_valid( - prompt_text, provider, llm_name, user_key, - az_deployment, az_endpoint, api_ver - ): + if not are_all_inputs_valid( + prompt_text, provider, llm_name, user_key, + az_endpoint, az_deployment, api_ver + ):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app.py(6 hunks)helpers/chat_helper.py(1 hunks)helpers/llm_helper.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-23T04:32:09.834Z
Learnt from: barun-saha
PR: barun-saha/slide-deck-ai#139
File: helpers/llm_helper.py:70-84
Timestamp: 2025-09-23T04:32:09.834Z
Learning: Hugging Face model support has been discontinued in the slide-deck-ai project, so PROVIDER_HUGGING_FACE should remain commented out in GlobalConfig.VALID_PROVIDERS and validation should continue rejecting 'hf' providers.
Applied to files:
helpers/llm_helper.pyapp.py
🧬 Code graph analysis (2)
helpers/llm_helper.py (1)
global_config.py (1)
GlobalConfig(16-198)
app.py (3)
global_config.py (1)
GlobalConfig(16-198)helpers/chat_helper.py (4)
StreamlitChatMessageHistory(30-46)ChatPromptTemplate(49-60)from_template(56-57)HumanMessage(16-20)helpers/llm_helper.py (1)
get_litellm_llm(238-300)
🪛 Ruff (0.13.1)
helpers/llm_helper.py
183-183: Avoid specifying long messages outside the exception class
(TRY003)
190-190: Avoid specifying long messages outside the exception class
(TRY003)
234-234: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
app.py
359-360: Possible SQL injection vector through string-based query construction
(S608)
🔇 Additional comments (3)
helpers/chat_helper.py (1)
23-27: Confirm Streamlit role name 'ai' is accepted; consider 'assistant' if not.Streamlit typically recognizes 'user' and 'assistant'. If 'ai' is unsupported, switch AIMessage to use role='assistant' while keeping compatibility with your history.
app.py (1)
295-306: LGTM on the chat-helper integration.Switch to StreamlitChatMessageHistory and template formatting is clean and minimal.
Please confirm st.chat_message(msg.type) works with 'ai' role in your deployed Streamlit version.
helpers/llm_helper.py (1)
195-217: Add timeout and OpenRouter base_url to LiteLLM request (prevents hangs, fixes OR routing).Currently no timeout is set and OpenRouter base URL isn’t provided.
Apply this diff:
# Prepare the request parameters request_params = { 'model': litellm_model, 'messages': messages, 'max_tokens': max_tokens, 'temperature': GlobalConfig.LLM_MODEL_TEMPERATURE, 'stream': True, + 'timeout': REQUEST_TIMEOUT, } @@ - if provider != GlobalConfig.PROVIDER_OLLAMA: + if provider != GlobalConfig.PROVIDER_OLLAMA: # For OpenRouter, pass API key as parameter if provider == GlobalConfig.PROVIDER_OPENROUTER: - request_params['api_key'] = api_key + request_params['api_key'] = api_key + request_params['base_url'] = OPENROUTER_BASE_URL elif provider == GlobalConfig.PROVIDER_AZURE_OPENAI: # For Azure OpenAI, pass credentials as parameters request_params['api_key'] = api_key request_params['azure_api_base'] = azure_endpoint_url request_params['azure_api_version'] = azure_api_version else: # For other providers, pass API key as parameter request_params['api_key'] = api_key
- Remove unsafe HTML styling per PR feedback - Simplify temp file deletion without exception handling - Use native Streamlit layout for button positioning
|
@barun-saha Rebased the latest changes and fixed review comments. Also tested along with the |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
app.py (2)
134-151: Style: add a second blank line after top‑level function.The project’s style guide asks for two blank lines after top‑level functions. Add one more after
reset_chat_history().def reset_chat_history(): @@ pptx_path.unlink() - +
365-373: Use the PDF_FILE_KEY constant instead of the magic string 'pdf_file'.This keeps keys consistent and avoids typos.
- f' {st.session_state["pdf_file"].name}' + f' {st.session_state[PDF_FILE_KEY].name}' @@ - f' {st.session_state["pdf_file"].name}' + f' {st.session_state[PDF_FILE_KEY].name}'helpers/llm_helper.py (4)
17-23: Remove unusedcompletionimport and variable.
completionisn’t used (you calllitellm.completion(...)), and keeping it complicates fallback logic.-try: - import litellm - from litellm import completion -except ImportError: - litellm = None - completion = None +try: + import litellm +except ImportError: + litellm = None
103-121: Azure validation: strengthen endpoint checks.
str(valid_url)will be truthy even for malformed URLs. Verify scheme and host, and keep other checks.- if provider == GlobalConfig.PROVIDER_AZURE_OPENAI: - valid_url = urllib3.util.parse_url(azure_endpoint_url) - all_status = all( - [azure_api_version, azure_deployment_name, str(valid_url)] - ) - return all_status + if provider == GlobalConfig.PROVIDER_AZURE_OPENAI: + valid_url = urllib3.util.parse_url(azure_endpoint_url) + if not (getattr(valid_url, 'scheme', None) and getattr(valid_url, 'host', None)): + return False + return all([azure_api_version, azure_deployment_name])
203-221: Uselogger.exceptionto capture traceback.Ensures stack traces are preserved on streaming failures.
- except Exception as e: - logger.error('Error in LiteLLM completion: %s', e) - raise + except Exception: + logger.exception('Error in LiteLLM completion') + raise
245-248: Consistent quotes per style guide.Project prefers single quotes. Minor, but change for consistency.
- if litellm is None: - logger.error("LiteLLM is not installed") + if litellm is None: + logger.error('LiteLLM is not installed')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/codeql.yml(1 hunks).gitignore(1 hunks)app.py(8 hunks)helpers/llm_helper.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-23T04:32:09.834Z
Learnt from: barun-saha
PR: barun-saha/slide-deck-ai#139
File: helpers/llm_helper.py:70-84
Timestamp: 2025-09-23T04:32:09.834Z
Learning: Hugging Face model support has been discontinued in the slide-deck-ai project, so PROVIDER_HUGGING_FACE should remain commented out in GlobalConfig.VALID_PROVIDERS and validation should continue rejecting 'hf' providers.
Applied to files:
app.pyhelpers/llm_helper.py
🧬 Code graph analysis (2)
app.py (4)
global_config.py (1)
GlobalConfig(16-198)helpers/chat_helper.py (4)
StreamlitChatMessageHistory(30-46)ChatPromptTemplate(49-60)from_template(56-57)HumanMessage(16-20)helpers/file_manager.py (1)
validate_page_range(48-73)helpers/llm_helper.py (2)
get_provider_model(37-78)get_litellm_llm(223-285)
helpers/llm_helper.py (1)
global_config.py (1)
GlobalConfig(16-198)
🪛 Ruff (0.13.1)
app.py
388-389: Possible SQL injection vector through string-based query construction
(S608)
helpers/llm_helper.py
168-168: Avoid specifying long messages outside the exception class
(TRY003)
175-175: Avoid specifying long messages outside the exception class
(TRY003)
219-219: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (3)
.github/workflows/codeql.yml (1)
12-99: LGTM — solid default CodeQL setup.Matrix, permissions, and steps look correct for Python. No blockers.
helpers/llm_helper.py (1)
55-72: Provider/model validation is good — keeps deprecated HF blocked.Validation against
GlobalConfig.VALID_PROVIDERSand empty-model checks are correct and aligned with the decision to discontinue HF.app.py (1)
333-341: st.chat_input accept_file usage is valid in Streamlit ≥1.44.0
st.chat_input returns a dict-like object with.textand.fileswhenaccept_file=True, so the existing code is correct if you’re on Streamlit 1.44.0 or newer. Pin your Streamlit version accordingly or, for older releases, fall back tost.file_uploader.Likely an incorrect or invalid review comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.gitignore (1)
147-149: Remove redundant.idea/**/.DS_Storeentry
.DS_Storealready ignores this file in all directories, and.idea/ignores the entire folder. The third rule is redundant.Apply this diff:
.DS_Store .idea/ -.idea/**/.DS_Store
Thanks, @sairampillai. Have added a few more comments. Also, I'm not very familiar with git rebase; I see the contributions from another PR appearing in the history here. That's fine I guess. Also, I resolved a merge conflict with After all review comments are addressed, I'll run the changes locally. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
helpers/llm_helper.py (4)
124-149: Align return type with behavior and docstringget_litellm_model_name may return None; update annotation and import Optional for accuracy.
-from typing import Tuple, Union, Iterator +from typing import Tuple, Union, Iterator, Optional @@ -def get_litellm_model_name(provider: str, model: str) -> str: +def get_litellm_model_name(provider: str, model: str) -> Optional[str]: @@ - :return: LiteLLM-compatible model name, or None if provider is not supported. + :return: LiteLLM-compatible model name, or None if provider is not supported.
226-228: Use logging.exception without explicit exception arg (TRY401)logging.exception already records the current exception; drop the redundant %s.
- logger.exception('Error in LiteLLM completion: %s', e) + logger.exception('Error in LiteLLM completion')
256-281: Annotate stream return type for claritySmall readability win: declare that stream yields strings.
- def stream(self, prompt: str): + def stream(self, prompt: str) -> Iterator[str]:
299-308: Remove HF sample to avoid confusionHF LLM support is discontinued; drop the '[hf]...' example from main.
inputs = [ '[co]Cohere', - '[hf]mistralai/Mistral-7B-Instruct-v0.2', '[gg]gemini-1.5-flash-002' ]Based on learnings
app.py (6)
215-223: Simplify provider extraction and use lazy loggingMinor cleanup; avoids duplicate parsing and aligns with logging style.
- provider_match = GlobalConfig.PROVIDER_REGEX.match(llm_provider_to_use) - if provider_match: - selected_provider = provider_match.group(1) - else: - # If regex doesn't match, try to extract provider from the beginning - selected_provider = llm_provider_to_use.split(' ')[0] if ' ' in llm_provider_to_use else llm_provider_to_use - logger.warning(f"Provider regex did not match for: {llm_provider_to_use}, using: {selected_provider}") + if provider_match: + selected_provider = provider_match.group(1) + else: + selected_provider = ( + llm_provider_to_use.split(' ')[0] + if ' ' in llm_provider_to_use else llm_provider_to_use + ) + logger.warning('Provider regex did not match for: %s, using: %s', + llm_provider_to_use, selected_provider)
481-487: Remove HF-specific exception pathHugging Face LLM support is discontinued; this catch and the import are dead weight here.
- except huggingface_hub.errors.ValidationError as ve: - handle_error( - f'An error occurred while trying to generate the content: {ve}' - '\nPlease try again with a significantly shorter input text.', - True - ) - returnAlso remove the unused import at the top:
-import huggingface_hubBased on learnings
498-509: Make “payment required” message provider-agnosticThis message still instructs users to use a Hugging Face token; replace with a generic provider message.
- if 'payment required' in _msg.lower(): + if 'payment required' in _msg.lower(): handle_error( - 'The available inference quota has exhausted.' - ' Please use your own Hugging Face access token. Paste your token in' - ' the input field on the sidebar to the left.' - '\n\nDon\'t have a token? Get your free' - ' [HF access token](https://huggingface.co/settings/tokens) now' - ' and start creating your slide deck! For gated models, you may need to' - ' visit the model\'s page and accept the terms or service.' - '\n\nAlternatively, choose a different LLM and provider from the list.', + f'Payment or quota issue for the selected provider ({provider}).' + ' Please verify your API key/billing limits for the provider account,' + ' or select a different model/provider and try again.', should_log=True )
364-372: Use the PDF_FILE_KEY constant instead of the hard-coded keyPrevents drift if the constant changes.
- st.text( - f'Extracting page {st.session_state["start_page"]} in' - f' {st.session_state["pdf_file"].name}' - ) + st.text( + f'Extracting page {st.session_state["start_page"]} in' + f' {st.session_state[PDF_FILE_KEY].name}' + ) @@ - st.text( - f'Extracting pages {st.session_state["start_page"]} to' - f' {st.session_state["end_page"]} in {st.session_state["pdf_file"].name}' - ) + st.text( + f'Extracting pages {st.session_state["start_page"]} to' + f' {st.session_state["end_page"]} in {st.session_state[PDF_FILE_KEY].name}' + )
612-612: Use logging.exception without explicit exception arg (TRY401)Remove the redundant %s argument.
- logger.exception('Caught a generic exception: %s', str(ex)) + logger.exception('Caught a generic exception while creating PPTX')
60-68: Clarify Azure arg names and orderingRename the Azure parameters in are_all_inputs_valid to azure_endpoint_url then azure_deployment_name, and update the sole call site in app.py (lines 398–401):
def are_all_inputs_valid( - user_prompt: str, - provider: str, - selected_model: str, - user_key: str, - azure_deployment_url: str = '', - azure_endpoint_name: str = '', + user_prompt: str, + provider: str, + selected_model: str, + user_key: str, + azure_endpoint_url: str = '', + azure_deployment_name: str = '', azure_api_version: str = '', ) -> bool: @@ if not llm_helper.is_valid_llm_provider_model( - provider, selected_model, user_key, - azure_endpoint_name, azure_deployment_url, azure_api_version + provider, selected_model, user_key, + azure_endpoint_url, azure_deployment_name, azure_api_version ): @@ - if not are_all_inputs_valid( - prompt_text, provider, llm_name, user_key, - az_deployment, az_endpoint, api_ver + if not are_all_inputs_valid( + prompt_text, provider, llm_name, user_key, + az_endpoint, az_deployment, api_ver ): return
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app.py(7 hunks)helpers/llm_helper.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-23T04:32:09.863Z
Learnt from: barun-saha
PR: barun-saha/slide-deck-ai#139
File: helpers/llm_helper.py:70-84
Timestamp: 2025-09-23T04:32:09.863Z
Learning: Hugging Face model support has been discontinued in the slide-deck-ai project, so PROVIDER_HUGGING_FACE should remain commented out in GlobalConfig.VALID_PROVIDERS and validation should continue rejecting 'hf' providers.
Applied to files:
app.pyhelpers/llm_helper.py
🧬 Code graph analysis (2)
app.py (3)
global_config.py (1)
GlobalConfig(16-198)helpers/chat_helper.py (4)
StreamlitChatMessageHistory(30-46)ChatPromptTemplate(49-60)from_template(56-57)HumanMessage(16-20)helpers/llm_helper.py (1)
get_litellm_llm(231-292)
helpers/llm_helper.py (1)
global_config.py (1)
GlobalConfig(16-198)
🪛 Ruff (0.13.1)
app.py
387-388: Possible SQL injection vector through string-based query construction
(S608)
612-612: Redundant exception object included in logging.exception call
(TRY401)
helpers/llm_helper.py
176-176: Avoid specifying long messages outside the exception class
(TRY003)
183-183: Avoid specifying long messages outside the exception class
(TRY003)
227-227: Redundant exception object included in logging.exception call
(TRY401)
254-254: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (2)
app.py (1)
224-228: Good: single UI error + stop, lazy loggingThis resolves the duplicate error display noted earlier and follows the logging guideline.
helpers/llm_helper.py (1)
189-210: Add timeout and OpenRouter base_urlAdd a per-request
timeout(e.g. 30s) torequest_paramsand setbase_urlforPROVIDER_OPENROUTERto"https://openrouter.ai/api/v1".request_params = { 'model': litellm_model, 'messages': messages, 'max_tokens': max_tokens, 'temperature': GlobalConfig.LLM_MODEL_TEMPERATURE, 'stream': True, + 'timeout': 30, } @@ - if provider != GlobalConfig.PROVIDER_OLLAMA: - # For OpenRouter, pass API key as parameter - if provider == GlobalConfig.PROVIDER_OPENROUTER: - request_params['api_key'] = api_key + if provider != GlobalConfig.PROVIDER_OLLAMA: + if provider == GlobalConfig.PROVIDER_OPENROUTER: + request_params['api_key'] = api_key + request_params['base_url'] = 'https://openrouter.ai/api/v1'
| selected_option = st.sidebar.selectbox( | ||
| label='2: Select a suitable LLM to use:\n\n(Gemini and Mistral-Nemo are recommended)', | ||
| options=[f'{k} ({v["description"]})' for k, v in GlobalConfig.VALID_MODELS.items()], | ||
| index=GlobalConfig.DEFAULT_MODEL_INDEX, | ||
| help=GlobalConfig.LLM_PROVIDER_HELP, | ||
| on_change=reset_api_key | ||
| ).split(' ')[0] | ||
| ) | ||
|
|
||
| # Extract provider key more robustly using regex | ||
| provider_match = GlobalConfig.PROVIDER_REGEX.match(selected_option) | ||
| if provider_match: | ||
| llm_provider_to_use = selected_option # Use full string for get_provider_model | ||
| else: | ||
| # Fallback: try to extract the key before the first space | ||
| llm_provider_to_use = selected_option.split(' ')[0] | ||
| logger.warning(f"Could not parse provider from selectbox option: {selected_option}") | ||
|
|
There was a problem hiding this comment.
Bug: description suffix leaks into provider/model string
selected_option includes a trailing description (e.g., "[az]azure/open-ai (…)" ). Passing it to get_provider_model yields an invalid model like "azure/open-ai (…)" and can break token limits lookups.
- selected_option = st.sidebar.selectbox(
+ selected_option = st.sidebar.selectbox(
label='2: Select a suitable LLM to use:\n\n(Gemini and Mistral-Nemo are recommended)',
options=[f'{k} ({v["description"]})' for k, v in GlobalConfig.VALID_MODELS.items()],
index=GlobalConfig.DEFAULT_MODEL_INDEX,
help=GlobalConfig.LLM_PROVIDER_HELP,
on_change=reset_api_key
)
-
- # Extract provider key more robustly using regex
- provider_match = GlobalConfig.PROVIDER_REGEX.match(selected_option)
- if provider_match:
- llm_provider_to_use = selected_option # Use full string for get_provider_model
- else:
- # Fallback: try to extract the key before the first space
- llm_provider_to_use = selected_option.split(' ')[0]
- logger.warning(f"Could not parse provider from selectbox option: {selected_option}")
+ # Always strip the description; keep only the "[xx]model" token
+ llm_provider_to_use = selected_option.split(' ', 1)[0]
+ provider_match = GlobalConfig.PROVIDER_REGEX.match(llm_provider_to_use)
+ if not provider_match:
+ logger.warning('Could not parse provider from selectbox option: %s', selected_option)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| selected_option = st.sidebar.selectbox( | |
| label='2: Select a suitable LLM to use:\n\n(Gemini and Mistral-Nemo are recommended)', | |
| options=[f'{k} ({v["description"]})' for k, v in GlobalConfig.VALID_MODELS.items()], | |
| index=GlobalConfig.DEFAULT_MODEL_INDEX, | |
| help=GlobalConfig.LLM_PROVIDER_HELP, | |
| on_change=reset_api_key | |
| ).split(' ')[0] | |
| ) | |
| # Extract provider key more robustly using regex | |
| provider_match = GlobalConfig.PROVIDER_REGEX.match(selected_option) | |
| if provider_match: | |
| llm_provider_to_use = selected_option # Use full string for get_provider_model | |
| else: | |
| # Fallback: try to extract the key before the first space | |
| llm_provider_to_use = selected_option.split(' ')[0] | |
| logger.warning(f"Could not parse provider from selectbox option: {selected_option}") | |
| selected_option = st.sidebar.selectbox( | |
| label='2: Select a suitable LLM to use:\n\n(Gemini and Mistral-Nemo are recommended)', | |
| options=[f'{k} ({v["description"]})' for k, v in GlobalConfig.VALID_MODELS.items()], | |
| index=GlobalConfig.DEFAULT_MODEL_INDEX, | |
| help=GlobalConfig.LLM_PROVIDER_HELP, | |
| on_change=reset_api_key | |
| ) | |
| # Always strip the description; keep only the "[xx]model" token | |
| llm_provider_to_use = selected_option.split(' ', 1)[0] | |
| provider_match = GlobalConfig.PROVIDER_REGEX.match(llm_provider_to_use) | |
| if not provider_match: | |
| logger.warning('Could not parse provider from selectbox option: %s', selected_option) |
| # Convert to LiteLLM model name | ||
| if provider == GlobalConfig.PROVIDER_AZURE_OPENAI: | ||
| from langchain_openai import AzureChatOpenAI | ||
|
|
||
| logger.debug('Getting LLM via Azure OpenAI: %s', model) | ||
|
|
||
| # The `model` parameter is not used here; `azure_deployment` points to the desired name | ||
| return AzureChatOpenAI( | ||
| azure_deployment=azure_deployment_name, | ||
| api_version=azure_api_version, | ||
| azure_endpoint=azure_endpoint_url, | ||
| temperature=GlobalConfig.LLM_MODEL_TEMPERATURE, | ||
| # max_tokens=max_new_tokens, | ||
| timeout=None, | ||
| max_retries=1, | ||
| api_key=api_key, | ||
| ) | ||
|
|
||
| if provider == GlobalConfig.PROVIDER_OPENROUTER: | ||
| # Use langchain-openai's ChatOpenAI for OpenRouter | ||
| from langchain_openai import ChatOpenAI | ||
| # For Azure OpenAI, use the deployment name as the model | ||
| # This is consistent with Azure OpenAI's requirement to use deployment names | ||
| if not azure_deployment_name: | ||
| raise ValueError("Azure deployment name is required for Azure OpenAI provider") | ||
| litellm_model = f'azure/{azure_deployment_name}' | ||
| else: | ||
| litellm_model = get_litellm_model_name(provider, model) | ||
|
|
There was a problem hiding this comment.
Guard against unsupported provider/model name
If get_litellm_model_name returns None, we currently pass model=None to LiteLLM.
else:
- litellm_model = get_litellm_model_name(provider, model)
+ litellm_model = get_litellm_model_name(provider, model)
+ if not litellm_model:
+ raise ValueError(
+ 'Unsupported provider/model; select a supported LLM from the list.'
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Convert to LiteLLM model name | |
| if provider == GlobalConfig.PROVIDER_AZURE_OPENAI: | |
| from langchain_openai import AzureChatOpenAI | |
| logger.debug('Getting LLM via Azure OpenAI: %s', model) | |
| # The `model` parameter is not used here; `azure_deployment` points to the desired name | |
| return AzureChatOpenAI( | |
| azure_deployment=azure_deployment_name, | |
| api_version=azure_api_version, | |
| azure_endpoint=azure_endpoint_url, | |
| temperature=GlobalConfig.LLM_MODEL_TEMPERATURE, | |
| # max_tokens=max_new_tokens, | |
| timeout=None, | |
| max_retries=1, | |
| api_key=api_key, | |
| ) | |
| if provider == GlobalConfig.PROVIDER_OPENROUTER: | |
| # Use langchain-openai's ChatOpenAI for OpenRouter | |
| from langchain_openai import ChatOpenAI | |
| # For Azure OpenAI, use the deployment name as the model | |
| # This is consistent with Azure OpenAI's requirement to use deployment names | |
| if not azure_deployment_name: | |
| raise ValueError("Azure deployment name is required for Azure OpenAI provider") | |
| litellm_model = f'azure/{azure_deployment_name}' | |
| else: | |
| litellm_model = get_litellm_model_name(provider, model) | |
| # Convert to LiteLLM model name | |
| if provider == GlobalConfig.PROVIDER_AZURE_OPENAI: | |
| # For Azure OpenAI, use the deployment name as the model | |
| # This is consistent with Azure OpenAI's requirement to use deployment names | |
| if not azure_deployment_name: | |
| raise ValueError("Azure deployment name is required for Azure OpenAI provider") | |
| litellm_model = f'azure/{azure_deployment_name}' | |
| else: | |
| litellm_model = get_litellm_model_name(provider, model) | |
| if not litellm_model: | |
| raise ValueError( | |
| 'Unsupported provider/model; select a supported LLM from the list.' | |
| ) |
🧰 Tools
🪛 Ruff (0.13.1)
183-183: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In helpers/llm_helper.py around lines 178 to 187, the code may call
get_litellm_model_name and end up with litellm_model being None which then gets
passed to LiteLLM; update the logic so after calling
get_litellm_model_name(provider, model) you check if the return value is None
and if so raise a clear ValueError (including provider and model in the message)
instead of allowing None to propagate; keep the existing Azure OpenAI branch (it
should still validate azure_deployment_name and set litellm_model =
f'azure/{azure_deployment_name}'), otherwise assign the non-None litellm_model
returned by get_litellm_model_name.
There was a problem hiding this comment.
@sairampillai Pls address this concern. If LiteLLM model is None, no further code execution here.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| help=GlobalConfig.LLM_PROVIDER_HELP, | ||
| on_change=reset_api_key | ||
| ).split(' ')[0] | ||
| ) |
There was a problem hiding this comment.
This copies the description along with the model name, leading to LiteLLM error. Append .split(' ')[0] to this line. See the original code.
| azure_deployment: str = '' | ||
| api_version: str = '' | ||
| else: | ||
| # The online LLMs |
There was a problem hiding this comment.
Let's replace selected_option with llm_provider_to_use. LLM model validations would follow, so no problem is somehow a name in wrong format appears here (which would be rejected).
| ) | ||
|
|
||
| # Extract provider key more robustly using regex | ||
| provider_match = GlobalConfig.PROVIDER_REGEX.match(selected_option) |
There was a problem hiding this comment.
Can you pls inspect lines 207 through 222? Why is PROVIDER_REGEX matched twice? Seems like duplicate steps.
| selected_provider = provider_match.group(1) | ||
| else: | ||
| # If regex doesn't match, try to extract provider from the beginning | ||
| selected_provider = llm_provider_to_use.split(' ')[0] if ' ' in llm_provider_to_use else llm_provider_to_use |
There was a problem hiding this comment.
Pls split lines longer than 100 chars using ( and ).
| else: | ||
| # If regex doesn't match, try to extract provider from the beginning | ||
| selected_provider = llm_provider_to_use.split(' ')[0] if ' ' in llm_provider_to_use else llm_provider_to_use | ||
| logger.warning(f"Provider regex did not match for: {llm_provider_to_use}, using: {selected_provider}") |
There was a problem hiding this comment.
Pls use lazy formatting with logs.
| # Convert to LiteLLM model name | ||
| if provider == GlobalConfig.PROVIDER_AZURE_OPENAI: | ||
| from langchain_openai import AzureChatOpenAI | ||
|
|
||
| logger.debug('Getting LLM via Azure OpenAI: %s', model) | ||
|
|
||
| # The `model` parameter is not used here; `azure_deployment` points to the desired name | ||
| return AzureChatOpenAI( | ||
| azure_deployment=azure_deployment_name, | ||
| api_version=azure_api_version, | ||
| azure_endpoint=azure_endpoint_url, | ||
| temperature=GlobalConfig.LLM_MODEL_TEMPERATURE, | ||
| # max_tokens=max_new_tokens, | ||
| timeout=None, | ||
| max_retries=1, | ||
| api_key=api_key, | ||
| ) | ||
|
|
||
| if provider == GlobalConfig.PROVIDER_OPENROUTER: | ||
| # Use langchain-openai's ChatOpenAI for OpenRouter | ||
| from langchain_openai import ChatOpenAI | ||
| # For Azure OpenAI, use the deployment name as the model | ||
| # This is consistent with Azure OpenAI's requirement to use deployment names | ||
| if not azure_deployment_name: | ||
| raise ValueError("Azure deployment name is required for Azure OpenAI provider") | ||
| litellm_model = f'azure/{azure_deployment_name}' | ||
| else: | ||
| litellm_model = get_litellm_model_name(provider, model) | ||
|
|
There was a problem hiding this comment.
@sairampillai Pls address this concern. If LiteLLM model is None, no further code execution here.
|
|
||
|
|
||
| def get_langchain_llm( | ||
| def get_litellm_model_name(provider: str, model: str) -> str: |
There was a problem hiding this comment.
Pls update the return type to Optional[str].
|
@barun-saha Will update and test in the next couple of days and push the changes |
3 participants
Implements Issue #100
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores
Style