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

Move code for Mephisto ACUTE-Evals into ParlAI #3002

Merged
merged 11 commits into from
Aug 25, 2020

Conversation

EricMichaelSmith
Copy link
Contributor

@EricMichaelSmith EricMichaelSmith commented Aug 24, 2020

Patch description
Moving the existing ACUTE-Eval Mephisto code into ParlAI

(Note: changed tests/test_code.py to exclude webapp folders, containing Javascript and no Python code)

Testing steps

python parlai_internal/projects/multimodal_blender/chats_with_images/run_acute_evals.py \
--pairings-filepath /checkpoint/ems/2020_multimodal_blender/chats_with_images/compiled/acuteeval_format__paired__full.jsonl \
--acute-eval-type human \
--architect-name heroku \
--requester-name <requester_name> \
--task-reward 1.25 \
--num-matchup-pairs 3000

Copy link
Contributor

@jxmsML jxmsML left a comment

Choose a reason for hiding this comment

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

lgtm.

help="Number of pairs per model matchup, default 2",
type=int,
)
parser.add_argument(
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need those flags as they are already included in the blueprint?

Copy link
Contributor

Choose a reason for hiding this comment

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

Having them here lets you set them from the command line. Hoping to make mephisto do this kind of thing automatically, but for now the transfer is manual.

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.

Looking good to me, though a few nits on some code that can now be safely removed or deleted.

# Code Instructions
Once you have installed [ParlAI](https://github.com/facebookresearch/ParlAI/#installing-parlai) and [Mephisto](https://github.com/facebookresearch/mephisto/blob/master/docs/quickstart.md), follow the instructions below.

The `run.py` script is designed to allow you to run this entire task from command line with an invocation like
Copy link
Contributor

Choose a reason for hiding this comment

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

It may make sense to rename example_script.py, or update this comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops - yes, changing

Comment on lines 97 to 103
# group.add_argument(
# '--task-config',
# type=dict,
# default=DEFAULT_TASK_CONFIG,
# help='dict with keys "hit_title", "hit_description", "hit_keywords", '
# 'determining how task is displayed on MTurk site',
# )
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can remove this arg

Comment on lines 153 to 159
# group.add_argument(
# '--softblock-list-path',
# dest="softblock_list_path",
# type=str,
# default=None,
# help='Path to list of workers to softblock, separated by line breaks',
# )
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this one as well

Comment on lines 419 to 438
# TODO(#98) this should be a util in a provider, not here
def softblock_workers(self):
"""
Softblock workers if necessary.
"""
if not self.opts["is_sandbox"] and self.opts["softblock_list_path"] is not None:
softblock_list = set()
with open(self.opts["softblock_list_path"]) as f:
for line in f:
softblock_list.add(line.strip())
logger.info(f"Will softblock {len(softblock_list):d} workers.")
for w in softblock_list:
try:
logger.info(f"Soft Blocking {w}\n")
self.manager.soft_block_worker(w)
except Exception as e:
logger.exception(
f"Did not soft block worker {w}: {e}", exc_info=True
)
time.sleep(0.1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This is dead code, can be removed.

help="Number of pairs per model matchup, default 2",
type=int,
)
parser.add_argument(
Copy link
Contributor

Choose a reason for hiding this comment

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

Having them here lets you set them from the command line. Hoping to make mephisto do this kind of thing automatically, but for now the transfer is manual.

@@ -0,0 +1,16 @@
## Description
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: remove this readme, it is outdated.

@@ -0,0 +1,5 @@
#!/usr/bin/env python3
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: no need for init in webapp directory (especially as you have modified the test already).

@EricMichaelSmith EricMichaelSmith merged commit 509c523 into master Aug 25, 2020
@EricMichaelSmith EricMichaelSmith deleted the mephisto-acute-evals branch August 25, 2020 19:07
@stephenroller stephenroller mentioned this pull request Sep 5, 2020
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.

4 participants