-
Notifications
You must be signed in to change notification settings - Fork 66
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
Add EvolComplexity
task
#415
Add EvolComplexity
task
#415
Conversation
Differs from the default `EvolInstruct` which will be refactored to be an evolution on top of existing instructions i.e. always expecting `seed_data` (requires modifications on top of the original implementation)
Return `inputs` where not properly formatted, and `yield` was running twice when `generate_answers=True`
Use `enum.Enum` instead of `enum.EnumType`
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.
Didn't review yet, but I'd place evol_complexity
under EvolInstruct
since it's based on it, rather than having it in a separate module, we can work on the imports later on i.e. from distilabel.tasks import EvolComplexity, EvolInstruct
@alvarobartt, I thought it was more structured when searching for tasks to have them in separate folders but I'll change the structure a bit. |
@alvarobartt feel free to review |
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.
Hi here! Some comments, but besides that I meant to nest the EvolComplexity
within a separate directory as evol_instruct/complexity/base.py
or something similar, to me is clearer, WDYT?
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 wouldn't mix the tests here, i.e. EvolInstruct
separately
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 think we'll create a lot of code/test-duplication for inherently testing the same things so I think it is better to keep it like this.
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 was thinking that as EvolComplexity
is just inheriting from EvolInstruct
and modifying the mutation_templates
, only that should be tested. Because the rest of it is just a duplication from EvolInstruct
, so there's no need to run the same tests twice, so a check on the mutation_templates
should be enough IMO. That would also help on keeping everything more organized IMO.
Co-authored-by: Alvaro Bartolome <alvaro@argilla.io>
Description
Implements EvolComplexity as a wrapper around EvolInstruct but with limited template options.
Closes #392