-
Notifications
You must be signed in to change notification settings - Fork 2.1k
unify turn annotation tasks (model chat and turn annotations static tasks) #4162
Conversation
…n annotations static task
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.
Yeah these improvements are super super useful to have, thanks a lot! Some comments about loose ends
parlai/crowdsourcing/tasks/model_chat/task_config/annotations_config.json
Show resolved
Hide resolved
parlai/crowdsourcing/tasks/turn_annotations_static/analysis/compile_results.py
Outdated
Show resolved
Hide resolved
parlai/crowdsourcing/tasks/turn_annotations_static/hydra_configs/conf/example.yaml
Show resolved
Hide resolved
parlai/crowdsourcing/tasks/turn_annotations_static/hydra_configs/conf/example.yaml
Show resolved
Hide resolved
parlai/crowdsourcing/tasks/turn_annotations_static/turn_annotations_blueprint.py
Outdated
Show resolved
Hide resolved
tests/crowdsourcing/tasks/model_chat/expected_states/final_chat_data.json
Show resolved
Hide resolved
@@ -41,13 +41,13 @@ function Checkboxes({ | |||
onChange={(evt) => { |
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.
@EricMichaelSmith @meganung I wonder if this logic work for non-checkbox inputtype, e.g. "radio" button,
suppose you click radio_bucket1, and then changed it to radio_bucket2,
I guess in this case, both radio_bucket1, and radio_bucket2 are True
, which is not exatly what we want
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.
shall we remove L31? and always set inputType = checkbox
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 does work if you specify type: radio in the config - i've done it in turn annotations static tasks at least. Radio buttons you can only select one so only one will be True
.
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.
Hmm interesting - @meganung in that case, do you expect that radio boxes will work correctly in the current version of model_chat as well, given this PR? I suppose I see two possibilities:
(1) we test out that radio buttons render correctly in the current version of model chat (i.e. with only one being checked at once as @jxmsML notes) and then add documentation about how to enable that to the README. In this case we should also add a unit test for radio buttons (or a TODO for a unit test at the very least, although a unit test is of course better)
(2) we just fix input_type = checkbox
for now, as @jxmsML suggests, and maybe add a TODO to add and test out radio box support for this task in the future
Either possibility is fine with me, but @meganung lemme know what you think :)
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.
good catch @jxmsML and thanks for the input @EricMichaelSmith -- I manually tested it and the frontend is fine (visually you can only check one) but our code for updating the data doesn't do that so ends up saving multiple different values as true if the user switches between selecting different radio buttons. I added a TODO for now since adding support for radio buttons in model chat task can be a separate PR.
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.
cool look good! @EricMichaelSmith just wonder if these frontend changes (e.g. checkbox type) that might affect final data logged has a test somewhere in crowdsourcing task (or will be catched?)
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.
@jxmsML Hmm, anything that affects the final data will break one of the tests, yes - I see that Megan fixed the tests in this PR, but it doesn't look like she needed to fix anything related to the checkbox type, so presumably the checkbox type doesn't get saved in the final data :)
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!! awesome job @meganung
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.
Yeah thanks a lot @meganung , this is super useful!
Patch description
Testing steps
Circle CI