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

fix: improve logic for detecting ChatFields #5667

Merged
merged 4 commits into from
Nov 6, 2024

Conversation

leiyre
Copy link
Member

@leiyre leiyre commented Nov 4, 2024

Check the role and content properties to validate the chat field

@leiyre leiyre requested a review from frascuchon November 4, 2024 15:07
Copy link

github-actions bot commented Nov 4, 2024

The URL of the deployed environment for this PR is https://argilla-quickstart-pr-5667-ki24f765kq-no.a.run.app

Comment on lines +48 to +49
role,
content,
Copy link
Member

Choose a reason for hiding this comment

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

We're losing info by keeping only the first entry of value on this line (which is a dictionary of feature). This can introduce errors later. (Not for handling on this PR) cc @damianpumar

Comment on lines +10 to +11
role?: string;
content?: string;
Copy link
Member

Choose a reason for hiding this comment

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

I don't fully understand this data structure but I think role and content are quite specific to be part of a general structure. Anyway, this is something to review with @damianpumar.

We can go on with this solution if everything is working well.

@frascuchon frascuchon merged commit d9dce94 into develop Nov 6, 2024
4 checks passed
@frascuchon frascuchon deleted the fix/chat-type-when-import-dataset branch November 6, 2024 11:46
frascuchon added a commit that referenced this pull request Nov 8, 2024
Check the role and content properties to validate the chat field

---------

Co-authored-by: Francisco Aranda <[email protected]>
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