Skip to content

Conversation

dafnapension
Copy link
Collaborator

@dafnapension dafnapension commented Sep 1, 2025

@dafnapension
Copy link
Collaborator Author

dafnapension commented Sep 1, 2025

An example from test_error_context_with_context_object:

image

and from test_error_context_without_object:

image

@dafnapension dafnapension changed the title A potential fix for messy error boxes when Error is other than UnitxtError A potential fix for messy error boxes when Error is KeyError Sep 1, 2025
@dafnapension dafnapension changed the title A potential fix for messy error boxes when Error is KeyError A potential fix for messy error box when Error is KeyError Sep 1, 2025
Copy link
Member

@elronbandel elronbandel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A very annoying bug indeed. I tried to fix it in the past but hit a wall: currently the new error context messages are not affecting the error trace, your fix does affect it. Im not sure what is more important good error message or good error trace, but if you could solve it in a way both are optimized it will be much better.

Maybe the fix should be not to raise KeyError to begin with?

@dafnapension dafnapension force-pushed the small_issue_with_error_box branch from 65522c8 to 061f86a Compare September 2, 2025 20:21
@dafnapension dafnapension marked this pull request as draft September 2, 2025 20:56
@dafnapension dafnapension force-pushed the small_issue_with_error_box branch from 061f86a to ddead88 Compare September 2, 2025 21:19
@dafnapension
Copy link
Collaborator Author

Hi @elronbandel ,
KeyError jumps off "by itself" (by python and not by unitxt) in cases like in here: a given string is not found in a dict (that the string is supposed to be one of its keys).

I changed to return an extension of KeyError that prints out its args not as a constant string, but obeying the \n -s.
As an extension, it does return true for isinstance(e, KeyError) and hence no change in test_error_utils.
It will affect a piece of code expecting this error in the form of if type(e) == KeyError:.

@dafnapension dafnapension marked this pull request as ready for review September 2, 2025 21:34
@elronbandel
Copy link
Member

So what does the error trace look like now? is it of the original error?

my suggestion was to wrap the problematic external code with:

try:
     # call to external code that raises key error
except KeyError as e:
    raise RuntimeError("Key was not found: {e}")

but this wrapping has to be write on top of the problematic code, so the error trace leads to the exact point of error.

@dafnapension dafnapension force-pushed the small_issue_with_error_box branch from ddead88 to 659d395 Compare September 3, 2025 18:26
@dafnapension
Copy link
Collaborator Author

Hi @elronbandel , I attached the full notebook for you to see both traces. Are these close enough?

@dafnapension
Copy link
Collaborator Author

HI @elronbandel ,
Effectively, I imbedded this piece of code that you suggested -- inside your error_context

I am not sure I understand whether you consider this "right on top of the problematic code,".
If not - then forget it. I think that exact trace is most important. More than a well organized error box.

@dafnapension dafnapension force-pushed the small_issue_with_error_box branch from cff8ba0 to c3ce6b4 Compare September 13, 2025 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants