-
Notifications
You must be signed in to change notification settings - Fork 389
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: create responses and suggestions with spans #4623
feat: create responses and suggestions with spans #4623
Conversation
for more information, see https://pre-commit.ci
…la-io/argilla into feat/create-span-question-from-sdk
…ecords-with-spans
for more information, see https://pre-commit.ci
…io/argilla into feat/suggest-records-with-spans
…/create-responses-with-spans
…/create-responses-with-spans
…sponses-with-spans
<!-- Thanks for your contribution! As part of our Community Growers initiative 🌱, we're donating Justdiggit bunds in your name to reforest sub-Saharan Africa. To claim your Community Growers certificate, please contact David Berenstein in our Slack community or fill in this form https://tally.so/r/n9XrxK once your PR has been merged. --> # Description This PR adds response and suggestion method helpers to simplify and unify how users may work with responses and suggestions. Instead of using response and suggestion schemas init methods, users may use `*.with_question_value` method to create responses and suggestions given a question and a response value. This way will introduce some extra data validation which is not available using init methods directly. For example, given the following dataset: ```python dataset = rg.FeedbackDataset( fields =[rg.TextField(name="text")], question=[ rg.TextQuestion(name="question-1", required=True), rg.RatingQuestion(name="question-2", values=[1, 2], required=True), rg.LabelQuestion(name="question-3", labels=["a", "b", "c"], required=True), rg.MultiLabelQuestion(name="question-4", labels=["a", "b", "c"], required=True), rg.RankingQuestion(name="question-5", values=["a", "b"], required=True), ] ) ``` users could create responses and suggestions as follows: ```python question_1 = dataset.question_by_name("question-1") question_2 = dataset.question_by_name("question-2") question_3 = dataset.question_by_name("question-3") question_4 = dataset.question_by_name("question-4") question_5 = dataset.question_by_name("question-5") record = rg.FeedbackRecord( fields={ "text": "This is a text value for field"}, responses=[ ResponseSchema(status="submitted") .with_question_value(question_1, value="answer") .with_question_value(question_2, value=0) .with_question_value(question_3, value="a") .with_question_value(question_4, value=["a", "b"]) .with_question_value(question_5, value=[{"rank": 1, "value": "a"}, {"rank": 2, "value": "b"}]) ], suggestions=[ SuggestionSchema.with_question_value(question_1, value="answer"), SuggestionSchema.with_question_value(question_2, value=0), SuggestionSchema.with_question_value(question_3, value="a"), SuggestionSchema.with_question_value(question_4, value=["a", "b"]), SuggestionSchema.with_question_value(question_5, value=[{"rank": 1, "value": "a"}, {"rank":2, "value": "b"}]) ] ) ``` **Type of change** (Please delete options that are not relevant. Remember to title the PR according to the type of change) - [ ] New feature (non-breaking change which adds functionality) - [ ] Refactor (change restructuring the codebase without changing functionality) - [ ] Improvement (change adding some improvement to an existing functionality) **How Has This Been Tested** (Please describe the tests that you ran to verify your changes. And ideally, reference `tests`) - [ ] Test A - [ ] Test B **Checklist** - [ ] I added relevant documentation - [ ] I followed the style guidelines of this project - [ ] I did a self-review of my code - [ ] I made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] I filled out [the contributor form](https://tally.so/r/n9XrxK) (see text above) - [ ] I have added relevant notes to the `CHANGELOG.md` file (See https://keepachangelog.com/) --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
for more information, see https://pre-commit.ci
<!-- Thanks for your contribution! As part of our Community Growers initiative 🌱, we're donating Justdiggit bunds in your name to reforest sub-Saharan Africa. To claim your Community Growers certificate, please contact David Berenstein in our Slack community or fill in this form https://tally.so/r/n9XrxK once your PR has been merged. --> # Description This PR integrates span questions with HF datasets, allowing exporting/importing datasets with span questions. Closes #4635 **Type of change** (Please delete options that are not relevant. Remember to title the PR according to the type of change) - [ ] New feature (non-breaking change which adds functionality) - [ ] Refactor (change restructuring the codebase without changing functionality) - [X] Improvement (change adding some improvement to an existing functionality) **How Has This Been Tested** (Please describe the tests that you ran to verify your changes. And ideally, reference `tests`) - [ ] Test A - [ ] Test B **Checklist** - [ ] I added relevant documentation - [ ] I followed the style guidelines of this project - [ ] I did a self-review of my code - [ ] I made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] I filled out [the contributor form](https://tally.so/r/n9XrxK) (see text above) - [ ] I have added relevant notes to the `CHANGELOG.md` file (See https://keepachangelog.com/) --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
value = [ | ||
{"start": s, "end": e, "label": l} | ||
for s, e, l in zip(value["start"], value["end"], value["label"]) | ||
] | ||
responses[user_id or "user_without_id"]["values"].update({question.name: {"value": value}}) |
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.
perhaps we should add else statements here to be sure it raises errors/has behaviour when we add question types?
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'm using the current approach to support span values mappings. I wouldn't add cross-cutting solution in the current version of the SDK. This is something that we can add for new SDK implementation
value = [ | ||
{"start": s, "end": e, "label": l} | ||
for s, e, l in zip(value["start"], value["end"], value["label"]) | ||
] |
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.
perhaps we should add else statements here to be sure it raises errors/has behaviour when we add question types?
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'm using the current approach to support span values mappings. I wouldn't add cross-cutting solution in the current version of the SDK. This is something that we can add for new SDK implementation
elif question.type == QuestionTypes.span: | ||
value = [ | ||
{"start": s, "end": e, "label": l} | ||
for s, e, l in zip(value["start"], value["end"], value["label"]) |
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.
in terms of human-readability, I also like to add the extracted text normally. Something likevalue["text"][value["start"]:value["end"]]
but perhaps this is difficult top map back into the correct format when calling from_huggingface
?
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 change is not hard to add, if it makes sense.
elif question.type == QuestionTypes.span: | ||
value = [ | ||
{"start": s, "end": e, "label": l} | ||
for s, e, l in zip(value["start"], value["end"], value["label"]) |
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.
in terms of human-readability, I also like to add the extracted text normally. Something likevalue["text"][value["start"]:value["end"]]
but perhaps this is difficult top map back into the correct format when calling from_huggingface
?
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 change is not hard to add, if it makes sense.
|
||
@validator("labels", always=True) | ||
def normalize_labels(cls, v: List[Union[str, SpanLabelOption]]) -> List[SpanLabelOption]: | ||
return [SpanLabelOption(value=label, text=label) if isinstance(label, str) else label for label in v] | ||
|
||
@validator("labels") |
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.
wasn't there a max for the labels too which we defined on the server side? perhaps we can use it here as well?
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.
Yes, this is not hard to implement.
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.
Anyway, this is not the current behaviour that exists for single and multi-label settings. In both cases, there is a min validation but not a max one.
Also, if we plan to support a configurable value for this, adding a hard validation here may introduce workflow problems. So, I will let as is.
score: Optional[confloat(ge=0.0, le=1.0)] = None | ||
|
||
@root_validator | ||
def check_span(cls, values): |
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 saw we already tested for larger than 0 but do we also validate if the span is within the overall text field, i.e., they are not larger than the used text?
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.
We're covering basic data validation from SDK, adding more complex validations is not quite simple to implement for this SDK. Since there are these kinds of validations from the server, and the rest of the suggestions/responses didn't include validations, I would keep it simple for this version.
" issue, unless you're planning to log the response in Argilla, as" | ||
" it will be automatically set to the active `user_id`.", | ||
) | ||
return v |
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.
perphaps we should return None
explicitly here? not 1
and not True
also pass the if-statement
and perhaps we require an explicit None
to set the user_id
automatically?
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 code was already here. I just moved it. I wouldn't change anything except the span feature. This can be tackled in the new SDK.
def to_server_payload(self, question_name_to_id: Dict[str, UUID]) -> Dict[str, Any]: | ||
"""Method that will be used to create the payload that will be sent to Argilla | ||
to create a `SuggestionSchema` for a `FeedbackRecord`.""" | ||
payload = self.dict(exclude_unset=True, include={"type", "score", "value", "agent"}) |
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.
to be clear, all of "type", "score", "value", "agent" are stored but not all of them are used, correct?
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.
'score' is used to filter records at least from UI. The rest are not used yet, even if they're available for searches.
This PR adds the `text` value for span suggestions and responses when publishing HF datasets. Commented by @davidberenstein1957 [here](#4623 (comment)) --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Description
This PR defines a basic flow to support responses and suggestions with span values.
Closes #4618
Type of change
(Please delete options that are not relevant. Remember to title the PR according to the type of change)
How Has This Been Tested
(Please describe the tests that you ran to verify your changes. And ideally, reference
tests
)Checklist
CHANGELOG.md
file (See https://keepachangelog.com/)