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: create responses and suggestions with spans #4623

Merged

Conversation

frascuchon
Copy link
Member

@frascuchon frascuchon commented Mar 4, 2024

Description

This PR defines a basic flow to support responses and suggestions with span values.

import spacy
from datasets import load_dataset

import argilla as rg
from argilla.client.feedback.schemas import SpanValueSchema
from argilla.client.feedback.schemas.responses import ResponseSchema, ResponseBuilder


api_url = ...
api_key = ...

rg.init(api_url, api_key)

nlp = spacy.load("en_core_web_sm")
labels = nlp.get_pipe("ner").labels

gutenberg_config = rg.FeedbackDataset(
    fields=[rg.TextField(name="text")],
    questions=[rg.SpanQuestion(name="spans", title="Name entities recognition", field="text", labels=labels)]
)

ds = gutenberg_config.push_to_argilla("gutenberg_spacy-ner", workspace="argilla")
hf_ds = load_dataset("argilla/gutenberg_spacy-ner", split="train")

user = rg.User.me()

def to_response_spans(doc) -> list[SpanValueSchema]:
    """Converts a spacy doc to a list of response spans."""
    return [
        SpanValueSchema(start=ent.start_char, end=ent.end_char,label=ent.label_)
        for ent in doc.ents
    ]

question = ds.question_by_name("spans")
records = [
    rg.FeedbackRecord(
        fields={"text": r["text"]},
        responses=[
            ResponseSchema(
                status="draft",
                user_id=user.id,
                values=[question.response(to_response_spans(nlp(r["text"])))]
            )
        ],
        suggestions=[
            question.suggestion(value=to_response_spans(nlp(r["text"])), agent="spacy", type="model")
        ]
    )
    for r in hf_ds
]
ds.add_records(records)

Closes #4618

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 (see text above)
  • I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)

frascuchon and others added 30 commits February 29, 2024 15:50
…la-io/argilla into feat/create-span-question-from-sdk
…io/argilla into feat/suggest-records-with-spans
@frascuchon frascuchon changed the title [WIP] feat: create responses with spans feat: create responses with spans Mar 5, 2024
@frascuchon frascuchon changed the title feat: create responses with spans feat: create responses and suggestions with spans Mar 5, 2024
@frascuchon frascuchon changed the title feat: create responses and suggestions with spans [WIP]feat: create responses and suggestions with spans Mar 6, 2024
frascuchon and others added 2 commits March 6, 2024 16:30
<!-- 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>
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Mar 6, 2024
@frascuchon frascuchon changed the title [WIP]feat: create responses and suggestions with spans feat: create responses and suggestions with spans Mar 6, 2024
frascuchon and others added 4 commits March 7, 2024 16:07
<!-- 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>
@frascuchon frascuchon merged commit dcb8e37 into feat/span-questions-support Mar 11, 2024
16 checks passed
@frascuchon frascuchon deleted the feat/create-responses-with-spans branch March 11, 2024 08:49
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}})

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?

Copy link
Member Author

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"])
]

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?

Copy link
Member Author

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"])

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?

Copy link
Member Author

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"])

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?

Copy link
Member Author

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")

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?

Copy link
Member Author

@frascuchon frascuchon Mar 11, 2024

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.

Copy link
Member Author

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):

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?

Copy link
Member Author

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

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?

Copy link
Member Author

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"})

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?

Copy link
Member Author

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.

frascuchon added a commit that referenced this pull request Mar 13, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: python sdk Indicates that an issue or pull request is related to the Python SDK language: python Pull requests or issues that update Python code severity: minor Indicates that the issue isn't urgent or blocking size:XL This PR changes 500-999 lines, ignoring generated files. team: backend Indicates that the issue or pull request is owned by the backend team type: enhancement Indicates new feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants