Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

Model chat: allow multiple final ratings #4276

Merged
merged 7 commits into from Jan 5, 2022

Conversation

EricMichaelSmith
Copy link
Contributor

Patch description
In the model chat crowdsourcing task, ask for and save multiple 1-to-5 ratings at the end of the HIT. Questions can be specified through mephisto.blueprint.final_rating_question, with multiple questions separated by a pipe character. Multiple ratings will be saved in the format '2|4|3'.

Testing steps
With 1 final question:

python parlai/crowdsourcing/tasks/model_chat/run.py \
mephisto.architect.port=3006 \
mephisto.task.maximum_units_per_worker=1000000 \
mephisto.task.task_name=${TASK_NAME} \
mephisto.blueprint.num_turns=2

with PR, 1 question

With multiple final questions:

python parlai/crowdsourcing/tasks/model_chat/run.py \
mephisto.architect.port=3006 \
mephisto.task.maximum_units_per_worker=1000000 \
mephisto.task.task_name=${TASK_NAME} \
mephisto.blueprint.num_turns=2 \
mephisto.blueprint.final_rating_question=\'Please\ rate\ your\ partner\ on\ a\ scale\ of\ 1-5.\|On\ a\ scale\ of\ 1\ to\ 5,\ what\ is\ your\ favorite\ number?\'

with PR, 2 questions

@EricMichaelSmith EricMichaelSmith marked this pull request as ready for review December 23, 2021 23:31
Copy link
Contributor

@JackUrb JackUrb left a comment

Choose a reason for hiding this comment

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

This is a cool additional feature! Happy to approve.

From an API design perspective, I wonder about the choice to |-separate your questions and output vs providing both as an array. I suppose the issue here is backwards-compatibility?

Comment on lines 83 to 94
let all_ratings_filled = true;
let rating = "";
for (let i = 0; i < ratings.length; i++) {
if (ratings[i] === "") {
all_ratings_filled = false;
}
if (i === 0) {
rating += ratings[i];
} else {
rating += "|" + ratings[i];
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be:

let all_ratings_filled = ratings.every((r) => r !== "")
let rating = ratings.join('|')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, great - thanks, changed

@EricMichaelSmith
Copy link
Contributor Author

This is a cool additional feature! Happy to approve.

From an API design perspective, I wonder about the choice to |-separate your questions and output vs providing both as an array. I suppose the issue here is backwards-compatibility?

Yeah, exactly - a bunch of projects seem to currently be relying on model chat, making breaking changes costly, and I'm not convinced of how often we'll want to use multiple final questions anyway. I'm definitely open to changing to the array syntax, which would indeed be more ideal, down the line

@EricMichaelSmith EricMichaelSmith merged commit 4f39183 into main Jan 5, 2022
@EricMichaelSmith EricMichaelSmith deleted the allow-multiple-final-ratings branch January 5, 2022 22:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants