Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve cli for feedback review #816

Merged
merged 31 commits into from
Jul 19, 2022
Merged

Conversation

Etesam913
Copy link
Contributor

@Etesam913 Etesam913 commented Jul 10, 2022

Summary

Improves the cli for the feedback review script by providing default values when asking for inputs, by adding colors sparingly, and by using tables.

When going through reviewed feedback, the feedback is all displayed in one table for each agent.

Example:

Text that has a toxicity > 0.5 is red
reviewed_feedback

I did not choose to filter by question in this case (I entered the default value of -1) and that is why there is > 1 questions in the tables.

In addition, I did not choose to filter by toxicity in this case and that is why the feedback with text, "I HATE YOU" is visible in the results.

When going through unreviewed feedback, each piece of feedback has a table as each piece of feedback can be set to be reviewed

Example

Screen Shot 2022-07-10 at 2 16 10 PM

Video

The video goes through the whole process. It showcases the questions asked and the default options

review_feedback_script.mov

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 10, 2022
@Etesam913 Etesam913 mentioned this pull request Jul 14, 2022
2 tasks
@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2022

Codecov Report

Merging #816 (919070d) into main (4c284b9) will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #816      +/-   ##
==========================================
+ Coverage   64.50%   64.56%   +0.05%     
==========================================
  Files         107      107              
  Lines        9281     9281              
==========================================
+ Hits         5987     5992       +5     
+ Misses       3294     3289       -5     
Impacted Files Coverage Δ
mephisto/abstractions/providers/mock/mock_unit.py 87.50% <ø> (ø)
mephisto/data_model/assignment.py 57.81% <0.00%> (-3.91%) ⬇️
mephisto/data_model/unit.py 77.59% <0.00%> (-0.55%) ⬇️
...tractions/architects/channels/websocket_channel.py 76.56% <0.00%> (+8.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c284b9...919070d. Read the comment docs.

Copy link

@s0mya s0mya left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on, It looks good to me! I only had a comment about python constants. I'm not sure about python best practices, so I'm sharing what I've seen in most other languages.

One high level thought I do have is if a cli is the best interface for this kind of operation? Is that what researchers prefer to use? What if this instead was some kind if an admin web app?

@@ -187,37 +215,34 @@ def main():
)
)

if see_unreviewed_feedback in no_response:
# Filter the toxicity feedback to get unreviewed feedback
if see_unreviewed_feedback == "r":
Copy link

Choose a reason for hiding this comment

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

what does "r" mean? It's not ideal to user hard-coded strings like this.
Does python have a concept of constants? If so, this could be something like:
if see_unreviewed_feedback == REVIEW_STATE.REVIEWED:

Copy link
Contributor

Choose a reason for hiding this comment

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

Good callout. This is doable with python Enums most easily:

import enum
...


class FeedbackReviewType(enum.Enum):
    """Types of flags"""

    REVIEWED = "r" 
    UNREVIEWED = "u"

...
   choices = FeedbackReviewType.list()
...

Same feedback can also be applied to #813.

)
elif see_unreviewed_feedback in yes_response:
# Filter the toxicity feedback to get reviewed feedback
elif see_unreviewed_feedback == "u":
Copy link

Choose a reason for hiding this comment

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

Same note as above.

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.

Generally this looks great! Somya's point is solid for better engineering, and should also likely be applied to #813. Approved pending that changeset

To the overall point of if this is the best UX, it's certainly the one that (when SSHed into a deploy server) researchers are used to. In the long run it'd be amazing to be able to create a full dashboard and include this workflow (@pringshia took a first stab at the core back here), but it hasn't been prioritized.

@@ -187,37 +215,34 @@ def main():
)
)

if see_unreviewed_feedback in no_response:
# Filter the toxicity feedback to get unreviewed feedback
if see_unreviewed_feedback == "r":
Copy link
Contributor

Choose a reason for hiding this comment

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

Good callout. This is doable with python Enums most easily:

import enum
...


class FeedbackReviewType(enum.Enum):
    """Types of flags"""

    REVIEWED = "r" 
    UNREVIEWED = "u"

...
   choices = FeedbackReviewType.list()
...

Same feedback can also be applied to #813.

mephisto/scripts/local_db/review_tips_for_task.py Outdated Show resolved Hide resolved
@JackUrb JackUrb changed the base branch from add-feedback-component to main July 19, 2022 18:03
@Etesam913 Etesam913 merged commit 98e5b3f into main Jul 19, 2022
@Etesam913 Etesam913 deleted the improve-cli-for-feedback-review branch July 29, 2022 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. ui/ux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants