Skip to content
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

[MCLOUD-4623] Add more detailed exception when user has uppercase in their example case but could potentially match the exampe type #1245

Closed
wants to merge 1 commit into from

Conversation

shitaoli-db
Copy link

As title, we want to show user a better exception, i.e. to give lowercase keys, when the user example contain upper case keys like "Prompt" that could potentially be fixed by using lower cases.

@shitaoli-db shitaoli-db force-pushed the shitaoli-db/MCLOUD-4623 branch 3 times, most recently from 051fcab to deefc88 Compare May 31, 2024 02:10
@shitaoli-db shitaoli-db requested a review from jjanezhang May 31, 2024 18:08
@shitaoli-db shitaoli-db force-pushed the shitaoli-db/MCLOUD-4623 branch from b7dd625 to 42fcd8d Compare May 31, 2024 20:27
fix
allow case insensitive exampple keys

tmp

fix

fix

revert

fix

fix formatting

fix

format

format
@shitaoli-db shitaoli-db force-pushed the shitaoli-db/MCLOUD-4623 branch from 81d426a to d1561d3 Compare June 1, 2024 00:01
Copy link
Collaborator

@dakinggg dakinggg left a comment

Choose a reason for hiding this comment

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

This PR seems a bit overkill to me. It adds code just for detecting one possible you may have messed up your data (out of the many possible ways), when the error message would already tell you fairly clearly what was wrong. If this is an error we've seen a handful of times, maybe we can just update the UnknownExampleTypeError to mention that you may want to check your casing as the keys are case sensitive.

@shitaoli-db
Copy link
Author

This PR seems a bit overkill to me. It adds code just for detecting one possible you may have messed up your data (out of the many possible ways), when the error message would already tell you fairly clearly what was wrong. If this is an error we've seen a handful of times, maybe we can just update the UnknownExampleTypeError to mention that you may want to check your casing as the keys are case sensitive.

Yeah, I was thinking the same at first place, I think another idea is no change in the LLM foundry but rather to only change on https://github.com/databricks-mosaic/mcloud/pull/4083 for user facing error. @jjanezhang WDYT on this?

@shitaoli-db
Copy link
Author

Closed for now since we agreeed that the change should be on mcloud side.

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