UN-2105 [FEAT] Include adapter name in error messages#1825
Open
pk-zipstack wants to merge 2 commits intomainfrom
Open
UN-2105 [FEAT] Include adapter name in error messages#1825pk-zipstack wants to merge 2 commits intomainfrom
pk-zipstack wants to merge 2 commits intomainfrom
Conversation
Preserve the user-facing adapter name (e.g., "Unstract Trial LLM") from the platform service response and include it in error messages across SDK1 adapter consumers (LLM, Embedding, VectorDB, X2Text). Previously, adapter names were discarded during config retrieval and errors only showed provider names or instance IDs, making it hard to identify which adapter caused the issue. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Summary by CodeRabbit
WalkthroughThe changes add adapter name tracking throughout the SDK to provide contextual information in error messages. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
for more information, see https://pre-commit.ci
Contributor
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
prompt-service/src/unstract/prompt_service/core/index_v2.py (1)
100-109:⚠️ Potential issue | 🟠 MajorPre-existing issue:
file_hashmay be undefined.Note: This is not introduced by this PR, but
file_hashis only assigned on line 81 whenfile_info.file_hashis falsy. Whenfile_info.file_hashis truthy, the variablefile_hashused on line 101 will be undefined, causing aNameError.This should likely be:
file_hash = file_info.file_hash if not file_hash: file_hash = fs.get_hash_from_file(path=file_info.file_path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prompt-service/src/unstract/prompt_service/core/index_v2.py` around lines 100 - 109, The variable file_hash can be undefined when file_info.file_hash is truthy; update the assignment logic in the code that populates index_key so that file_hash is always set from file_info.file_hash first (e.g., file_hash = file_info.file_hash) and only call fs.get_hash_from_file(path=file_info.file_path) when that value is falsy, then use that ensured file_hash when building index_key (references: file_hash, file_info, fs.get_hash_from_file, index_key, self.chunking_config.chunk_size/ chunk_overlap).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@unstract/sdk1/src/unstract/sdk1/embedding.py`:
- Around line 100-105: The async embedding error paths still call
parse_litellm_err with only provider values; update those calls to include the
adapter display string like the sync path does. Compute adapter_info using
self._adapter_name and self.adapter.get_name() (same logic as the shown
adapter_info variable) and pass adapter_info into parse_litellm_err instead of
the provider-only argument in all async error raises (search for
parse_litellm_err in this module, e.g., the async embedding functions around the
current raise and the other occurrences referenced). Ensure the raise statements
use "raise parse_litellm_err(e, adapter_info) from e" so adapter context is
included.
In `@unstract/sdk1/src/unstract/sdk1/llm.py`:
- Around line 266-273: The acomplete error path is building messages that only
include the provider name and drops the adapter instance name; update the error
construction in the acomplete handler to use the same adapter_info logic as
elsewhere (use self._adapter_name when present, formatted as
"'{self._adapter_name}' ({self.adapter.get_provider()})", else fall back to
"'{self.adapter.get_provider()}'") and use that adapter_info when composing
error_msg (same change should be applied to the other occurrence around lines
346-353) so async callers receive the adapter instance name in the error text.
---
Outside diff comments:
In `@prompt-service/src/unstract/prompt_service/core/index_v2.py`:
- Around line 100-109: The variable file_hash can be undefined when
file_info.file_hash is truthy; update the assignment logic in the code that
populates index_key so that file_hash is always set from file_info.file_hash
first (e.g., file_hash = file_info.file_hash) and only call
fs.get_hash_from_file(path=file_info.file_path) when that value is falsy, then
use that ensured file_hash when building index_key (references: file_hash,
file_info, fs.get_hash_from_file, index_key, self.chunking_config.chunk_size/
chunk_overlap).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff0f61a0-7b62-4b96-ae66-eecf6097d69f
📒 Files selected for processing (9)
prompt-service/src/unstract/prompt_service/core/index_v2.pyunstract/sdk1/src/unstract/sdk1/constants.pyunstract/sdk1/src/unstract/sdk1/embedding.pyunstract/sdk1/src/unstract/sdk1/index.pyunstract/sdk1/src/unstract/sdk1/llm.pyunstract/sdk1/src/unstract/sdk1/platform.pyunstract/sdk1/src/unstract/sdk1/utils/indexing.pyunstract/sdk1/src/unstract/sdk1/vector_db.pyunstract/sdk1/src/unstract/sdk1/x2txt.py
Comment on lines
+100
to
+105
| adapter_info = ( | ||
| f"{self._adapter_name} ({self.adapter.get_name()})" | ||
| if self._adapter_name | ||
| else self.adapter.get_name() | ||
| ) | ||
| raise parse_litellm_err(e, adapter_info) from e |
Contributor
There was a problem hiding this comment.
Async embedding errors still miss adapter-name context.
You updated sync paths, but Line 153 and Line 166 still pass provider-only values to parse_litellm_err. That leaves async consumers outside the UN-2105 behavior.
Proposed fix
class Embedding:
+ def _adapter_info(self) -> str:
+ return (
+ f"{self._adapter_name} ({self.adapter.get_name()})"
+ if self._adapter_name
+ else self.adapter.get_name()
+ )
+
@@
except Exception as e:
- adapter_info = (
- f"{self._adapter_name} ({self.adapter.get_name()})"
- if self._adapter_name
- else self.adapter.get_name()
- )
- raise parse_litellm_err(e, adapter_info) from e
+ raise parse_litellm_err(e, self._adapter_info()) from e
@@
except Exception as e:
- adapter_info = (
- f"{self._adapter_name} ({self.adapter.get_name()})"
- if self._adapter_name
- else self.adapter.get_name()
- )
- raise parse_litellm_err(e, adapter_info) from e
+ raise parse_litellm_err(e, self._adapter_info()) from e
@@
except Exception as e:
- adapter_info = (
- f"{self._adapter_name} ({self.adapter.get_name()})"
- if self._adapter_name
- else self.adapter.get_name()
- )
- raise parse_litellm_err(e, adapter_info) from e
+ raise parse_litellm_err(e, self._adapter_info()) from e
@@
except Exception as e:
- provider_name = f"{self.adapter.get_name()}"
- raise parse_litellm_err(e, provider_name) from e
+ raise parse_litellm_err(e, self._adapter_info()) from e
@@
except Exception as e:
- provider_name = f"{self.adapter.get_name()}"
- raise parse_litellm_err(e, provider_name) from e
+ raise parse_litellm_err(e, self._adapter_info()) from eAlso applies to: 119-124, 136-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@unstract/sdk1/src/unstract/sdk1/embedding.py` around lines 100 - 105, The
async embedding error paths still call parse_litellm_err with only provider
values; update those calls to include the adapter display string like the sync
path does. Compute adapter_info using self._adapter_name and
self.adapter.get_name() (same logic as the shown adapter_info variable) and pass
adapter_info into parse_litellm_err instead of the provider-only argument in all
async error raises (search for parse_litellm_err in this module, e.g., the async
embedding functions around the current raise and the other occurrences
referenced). Ensure the raise statements use "raise parse_litellm_err(e,
adapter_info) from e" so adapter context is included.
Comment on lines
+266
to
273
| adapter_info = ( | ||
| f"'{self._adapter_name}' ({self.adapter.get_provider()})" | ||
| if self._adapter_name | ||
| else f"'{self.adapter.get_provider()}'" | ||
| ) | ||
| error_msg = ( | ||
| f"Error from LLM provider '{self.adapter.get_provider()}': " | ||
| f"Error from LLM adapter {adapter_info}: " | ||
| f"{strip_litellm_prefix(str(e))}" |
Contributor
There was a problem hiding this comment.
acomplete still emits provider-only errors.
Line 406 still builds "Error from LLM provider ..." and drops adapter instance name context, so async callers don’t get the same UN-2105 behavior.
Proposed fix
@@
+ def _adapter_info(self) -> str:
+ return (
+ f"'{self._adapter_name}' ({self.adapter.get_provider()})"
+ if self._adapter_name
+ else f"'{self.adapter.get_provider()}'"
+ )
+
@@
- adapter_info = (
- f"'{self._adapter_name}' ({self.adapter.get_provider()})"
- if self._adapter_name
- else f"'{self.adapter.get_provider()}'"
- )
+ adapter_info = self._adapter_info()
@@
- adapter_info = (
- f"'{self._adapter_name}' ({self.adapter.get_provider()})"
- if self._adapter_name
- else f"'{self.adapter.get_provider()}'"
- )
+ adapter_info = self._adapter_info()
@@
- error_msg = (
- f"Error from LLM provider '{self.adapter.get_provider()}': "
- f"{strip_litellm_prefix(str(e))}"
- )
+ error_msg = (
+ f"Error from LLM adapter {self._adapter_info()}: "
+ f"{strip_litellm_prefix(str(e))}"
+ )Also applies to: 346-353
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@unstract/sdk1/src/unstract/sdk1/llm.py` around lines 266 - 273, The acomplete
error path is building messages that only include the provider name and drops
the adapter instance name; update the error construction in the acomplete
handler to use the same adapter_info logic as elsewhere (use self._adapter_name
when present, formatted as "'{self._adapter_name}'
({self.adapter.get_provider()})", else fall back to
"'{self.adapter.get_provider()}'") and use that adapter_info when composing
error_msg (same change should be applied to the other occurrence around lines
346-353) so async callers receive the adapter instance name in the error text.
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.
What
Why
How
PlatformHelper._get_adapter_configuration(), the adapter name was already fetched from the platform service but immediately discarded viapop(). Now it is stored back in the config dict under a private key_adapter_name.Error from LLM provider 'azureopenai': ...Error from LLM adapter 'Unstract Trial LLM' (azureopenai): ...index.py,utils/indexing.py, prompt-serviceindex_v2.py) strip the_adapter_namekey before hashing to maintain backward compatibility with existing document index keys.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
_adapter_namekey is stripped before index key hashing, so existing indexed documents are unaffected. The only user-visible change is richer error messages that now include the adapter instance name alongside the provider name.Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
Screenshots
N/A
Checklist
I have read and understood the Contribution Guidelines.