-
Notifications
You must be signed in to change notification settings - Fork 364
Adds config tempaltes #295
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
Conversation
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.
The templates folder mixes up a lot of items of different logic level, we'll have to disuss the structure IRL.
Do we really need "NLITaskConfig, CopaTaskConfig, BoolQATaskConfig", etc? I think it would be better to aim for a general structure instead of having lots of small ones
I did remove the BoolQATask, for the rest I think they have their place as they have some logic in them. Secondly I decided that I won't create a question-answer template as one can just use multichoice with cf to achieve just that |
78f3518
to
22eeddb
Compare
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.
Thanks for the PR!
Some comments:
- avoid too many layers of nested function: in
continuation
, you nest 3 functions in one other function over about 100 lines of code, it's not super clear - in general, you also need to be careful to not change the naming conventions of the lib (for example, we use
choices
and you usedoptions
), it would be best if you could rename to fit our naming convention. - other comments are mostly nits/need for doc/comment suggestions/...
instruction: NotRequired[str] | ||
|
||
|
||
# Python too dumb to do fancy inference :( |
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.
Remove or convert to actionable todo
translation_literals = TRANSLATION_LITERALS[language] | ||
adapter_fn = create_adapter_from_dict(adapter) if isinstance(adapter, dict) else adapter # type: ignore | ||
|
||
def nli_natural_prompt(line: dict, task_name: str): |
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.
add docstring explaining the diff between the 2 prompt functions
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.
It should be clear from the format. The second function uses the natural function for CF formulation
elif label == "contradiction": | ||
return translation_literals.no | ||
elif label == "neutral": | ||
return translation_literals.also |
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.
else raise error?
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.
That can't happen as long as you are respecting the typing, you don't that's on you
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 would not underestimate the creativity of some users ^^ and add an error message - but program will fail later anyway so up to you
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.
Well it's like calling load_datset(False), it's undefine behaviour, so it's user's problem.
from lighteval.utils.utils import as_list | ||
|
||
|
||
CONTINUATION_QUERY_CF = "{instruction}{context}" |
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.
Would be good to explain a bit what the diff is between continuation and multichoice
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 did so in the function docstring
Co-authored-by: Clémentine Fourrier <[email protected]>
Co-authored-by: Clémentine Fourrier <[email protected]>
Co-authored-by: Clémentine Fourrier <[email protected]>
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.
LGTM, super nice, thanks for the changes
This reverts commit b014b50.
This is PR, which will bring easier way to task prompts, which are language-aware.
Following templates are create