-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Mephisto QA data collection #3318
Mephisto QA data collection #3318
Conversation
Merge with latest code
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.
Wow, I love this - thanks @vaibhavad for adding this! It'll be a great addition to our set of Mephisto crowdsourcing tasks. I've added a bunch of comments, mostly relating to standard ParlAI and parlai.crowdsourcing
conventions, but it looks like the major components are all here. To address your points below:
- Yeah, there's a standard ParlAI way to load data - see my comments on this, but happy to explain further if you'd like.
- I think showing evidence would be a great add-on here: I'll defer to @JackUrb and @pringshia for advice on configuring the front-end code for this
Great job!
parlai/crowdsourcing/tasks/qa_data_collection/conf/dataloader_squad.yaml
Outdated
Show resolved
Hide resolved
parlai/crowdsourcing/tasks/qa_data_collection/conf/onboarding_example.yaml
Outdated
Show resolved
Hide resolved
parlai/crowdsourcing/tasks/qa_data_collection/task_description.html
Outdated
Show resolved
Hide resolved
parlai/crowdsourcing/tasks/qa_data_collection/task_description.html
Outdated
Show resolved
Hide resolved
Hi @vaibhavad, thanks for bringing this together! Definitely agreed that adding more context to a task such that it's not all inline in the chat would be great. In While it'd be a great enhancement for sure, I don't think it should stop this from getting merged as is, as this was the level of usability we had for the original task. Can always open a follow-up PR if you want to contribute more! |
Hi @JackUrb, Thanks a lot for your comments. I'll explore overriding components in |
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 so much for all of the refactors on this! Approving now. It looks like Jack is out of the office until next week, but it seems like he agrees that the remaining improvements can be done in a follow-up PR, so it's fine with me if you merge in :)
Thanks @EricMichaelSmith for all the constructive feedback throughout. Feels good to have my first contribution. I don't think I have permission to merge. You or @JackUrb can merge it if no more changes are required. |
Okay cool! It looks like some of the CI checks are failing (linting and stuff) - this is pretty minor, but when I have time today I'll fix those up before merging in (unless you want to take a crack at them first, totally up to you) |
Description
Example of QA Data collection task using Mephisto
Testing steps
Discussion
There are two major points where this can be improved -
bootstrap-chat
package.@EricMichaelSmith Please review it and suggest changes.