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

add emoji input functionality to model chat crowdsourcing task #4666

Merged
merged 2 commits into from
Jul 18, 2022

Conversation

basilnajjar
Copy link
Contributor

@basilnajjar basilnajjar commented Jul 13, 2022

Users can now add emojis to the text box by using the emoji picker. To show the emoji picker, you need to set the emoji_picker flag to true (it's set to false by default).

Test plan:

npm install react-input-emoji
python parlai/crowdsourcing/tasks/model_chat/run.py mephisto.blueprint.emoji_picker=true

Screen Shot 2022-07-13 at 5 00 21 PM

@EricMichaelSmith
Copy link
Contributor

Hmm - can you run python parlai/crowdsourcing/tasks/model_chat/run.py, make sure it still works, and then add that to the PR description as part of the test plan? Since we don't have any frontend integration tests currently, this manual step is a good check to make sure that the task still works as expected :)

@basilnajjar
Copy link
Contributor Author

Hmm - can you run python parlai/crowdsourcing/tasks/model_chat/run.py, make sure it still works, and then add that to the PR description as part of the test plan? Since we don't have any frontend integration tests currently, this manual step is a good check to make sure that the task still works as expected :)

Done.

@EricMichaelSmith
Copy link
Contributor

Hmm - I'm a bit nervous about changing the model chat task to always show the emoji picker, because many of our models can't handle emojis, and so this might mean that the future conversations that we collect will contain largely unparseable emojis. Could you try adding a Hydra flag to conditionally show the emoji picker and default it to False? Another issue is that it looks like this change means that developers will always need to run npm install react-input-emoji to run the model chat task, and I'm loath to require that if most people won't need to be using the emoji picker

@basilnajjar
Copy link
Contributor Author

Hmm - I'm a bit nervous about changing the model chat task to always show the emoji picker, because many of our models can't handle emojis, and so this might mean that the future conversations that we collect will contain largely unparseable emojis. Could you try adding a Hydra flag to conditionally show the emoji picker and default it to False? Another issue is that it looks like this change means that developers will always need to run npm install react-input-emoji to run the model chat task, and I'm loath to require that if most people won't need to be using the emoji picker

See updated description above

Copy link
Contributor

@EricMichaelSmith EricMichaelSmith left a comment

Choose a reason for hiding this comment

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

Hmm - the one thing I'm still a bit worried about is that people will now have to install the emoji picker tool in order to use model chat, even if the emoji picker flag is False. But since we always have to install a bunch of packages anyway to run any task, I'm not convinced that one more will be a huge problem. Thus, I'm willing to go ahead and approve this - if this extra package becomes a huge problem, we can reassess later. Great addition!

@basilnajjar basilnajjar merged commit 2f42a5b into main Jul 18, 2022
@basilnajjar basilnajjar deleted the add-emoji-input-functionality-to-model-chat-task branch July 18, 2022 15:11
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.

3 participants