clp-research / clembench

A Framework for the Systematic Evaluation of Chat-Optimized Language Models as Conversational Agents and an Extensible Benchmark
MIT License
19 stars 26 forks source link

player calls method to ensure alternating roles in messages history #77

Closed phisad closed 2 months ago

phisad commented 2 months ago

~I added this to Player because this is the only place to have this fixed without touching all the backend implementations. An alternative would be to add a decorator to generate_response.~

I added the decorator and adjusted all generate_response methods, because decorators are not inherited. This is actually now a useful hook to do all kinds of things. :)

Now there will be a warning when this happens in the clembench.log (not in the stdout). In addition I left the same procedure in the hf_local_api to ensure backwards compatibility. But this can probably be removed later on.

phisad commented 2 months ago

Note that I changed the concatenation from whitespace " " to "\n\n" Maybe discuss with JJ if this is okey! @sherzod-hakimov

sherzod-hakimov commented 2 months ago

@phisad great. I'm testing it now.

Does it also make sense to fix this in taboo where it originates?

Gnurro commented 2 months ago

Note that I changed the concatenation from whitespace " " to "\n\n" Maybe discuss with JJ if this is okey! @sherzod-hakimov

I don't think adding more newlines like that is a good idea. It not only breaks some chat formats, it also adds more newlines to context in general, making the models more likely to generate them (which we are already struggling with with some models).

I do like the addition of a warning in the logs, I was thinking of adding that as well.