-
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
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
|
@Prigoistic Can we clean up the unintended changes in examples so we can merge this PR? Also, rebase with main |
|
@anistark i already cleaned the unintended changes that we talked about in last commit |
Issue Link / Problem Description
EvaluationResultnot defined, because it was imported only undert.TYPE_CHECKING. Intermittent LlamaIndex execution failures also led toIndexErrorduring result collection due to mismatched lengths.Changes Made
EvaluationResultat runtime fromragas.dataset_schemainsrc/ragas/integrations/llama_index.py.IndexErrorduring dataset augmentation.Testing
How to Test
uv run pip install -e . -e ./examplesquery_engineandEvaluationDataset: docsn=1(or unset) to avoid “n values greater than 1 not support” warnings.NameErrorand withoutIndexErrorduring result collection.uv run ruff check .References
Screenshots/Examples (if applicable)