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

feat: add target_context to dataset columns #266

Merged
merged 3 commits into from
May 21, 2024
Merged

Conversation

oyangz
Copy link
Contributor

@oyangz oyangz commented Apr 26, 2024

Issue #, if available:

Description of changes:

Update: Initially we planned on the target_context dataset column taking list of strings. This does not work with ray operations such as map_batches due to issues including ray-project/ray#39559 and other unsupported data type errors. Thus the target_context dataset column has been modified to take a string, and we will use string concatenation when there are multiple target contexts, similar to the existing target_output field.

Description (updated):

  • Adds support for loading target_context for evaluation of RAG "ground truth" context provided in a dataset. The target_context for each dataset sample is a list of string.
  • Modifies json_parser to accept lists of strings by updating JMESPath output validation and string casting.
  • Adds unit tests for JSON and JSONLINES cases.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@oyangz oyangz changed the title feat: add target_context to data_loaders feat: add target_context to dataset columns Apr 26, 2024
danielezhu
danielezhu previously approved these changes Apr 26, 2024
Copy link
Contributor

@danielezhu danielezhu left a comment

Choose a reason for hiding this comment

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

Non-blocking thought: is target context the only column type that we anticipate being a special case (ie requiring different logic for json parsing)? If we end up having other columns that are similar in behavior to target context, we will want to group these column types together.

franluca
franluca previously approved these changes Apr 29, 2024
@franluca
Copy link
Contributor

Non-blocking thought: is target context the only column type that we anticipate being a special case (ie requiring different logic for json parsing)? If we end up having other columns that are similar in behavior to target context, we will want to group these column types together.

Just to double check, is the different logic there because we expect the context to be a list? If so:

  1. what happens if it is actually not a list?
  2. this is pretty similar to the situation with multiple ground truths, which we had the workaround to concatenate together using the "". Is this no longer necessary? if so, I find it a bit undesirable to have different treatment for the same case.

@xiaoyi-cheng
Copy link
Contributor

this is pretty similar to the situation with multiple ground truths, which we had the workaround to concatenate together using the "". Is this no longer necessary? if so, I find it a bit undesirable to have different treatment for the same case.

Interesting. Let's hold off this PR and discuss offline. Maybe we should keep target_context as a string and concatenate the strings together.

@xiaoyi-cheng xiaoyi-cheng changed the title feat: add target_context to dataset columns [DO NOT MERGE] feat: add target_context to dataset columns May 1, 2024
@oyangz oyangz dismissed stale reviews from franluca and danielezhu via 079bc9b May 20, 2024 18:09
@oyangz oyangz changed the title [DO NOT MERGE] feat: add target_context to dataset columns feat: add target_context to dataset columns May 20, 2024
@oyangz oyangz merged commit 6190b49 into aws:main May 21, 2024
3 checks passed
@oyangz oyangz deleted the target_context branch May 21, 2024 16:02
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.

5 participants