-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fixed: NameError during evalutation of llamaindex query engine #2331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fixed: NameError during evalutation of llamaindex query engine #2331
Conversation
hey @Prigoistic I've fixed the CI - could you take a look and see if everything looks good? |
retrieved_contexts.append([n.node.text for n in r.source_nodes]) | ||
# Handle failed jobs which are recorded as NaN in the executor | ||
if isinstance(r, float) and math.isnan(r): | ||
responses.append("") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to fail loudly than silently.
If we still need to pass through, better to keep None
. The later metrics can skip None
or handle them explicitly.
responses.append(None)
retrieved_contexts.append(None)
logger.warning(f"Query engine failed for query {i}: '{queries[i]}'")
retrieved_contexts.append([]) | ||
else: | ||
# Cast to LlamaIndex Response type for proper type checking | ||
response = t.cast("LlamaIndexResponse", r) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This'll be hard on type hints.
Probably better to take from llama_index.core.base.response.schema import Response as LlamaIndexResponse
else: | ||
# Cast to LlamaIndex Response type for proper type checking | ||
response = t.cast("LlamaIndexResponse", r) | ||
responses.append(response.response or "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this more explicit?
responses.append(response.response if response.response is not None else "")
@jjmachan yes everything looks good to me |
I see no conflicts so far and all the checks has been passed too, you can go further and merge this :) |
@Prigoistic I don't see any changes to the comments. |
@anistark oh shoot i forgot to push the changes gimme a sec |
@anistark pushed the changes as per the comments :) please check it once |
… of import in eval.py
Issue Link / Problem Description
EvaluationResult
not defined, because it was imported only undert.TYPE_CHECKING
. Intermittent LlamaIndex execution failures also led toIndexError
during result collection due to mismatched lengths.Changes Made
EvaluationResult
at runtime fromragas.dataset_schema
insrc/ragas/integrations/llama_index.py
.IndexError
during dataset augmentation.Testing
How to Test
uv run pip install -e . -e ./examples
query_engine
andEvaluationDataset
: docsn=1
(or unset) to avoid “n values greater than 1 not support” warnings.NameError
and withoutIndexError
during result collection.uv run ruff check .
References
Screenshots/Examples (if applicable)