Skip to content
This repository was archived by the owner on Oct 11, 2023. It is now read-only.

Mephisto LIGHT worker task#283

Closed
JackUrb wants to merge 1367 commits intomainfrom
mephisto-worker-task
Closed

Mephisto LIGHT worker task#283
JackUrb wants to merge 1367 commits intomainfrom
mephisto-worker-task

Conversation

@JackUrb
Copy link
Copy Markdown
Contributor

@JackUrb JackUrb commented Mar 25, 2022

Overview

Following up on #282, this PR updates the task frontend introduced in #281 to now be in a Mephisto-task format. Note, this now requires using Mephisto version 1.0.0.

Implementation

Due to how Mephisto tasks build, I had to rearrange a number of folders, configure webpack, change react versions, etc. to get things running. That's most of the churn of this PR. Additionally:

  • Replaces the AppRouter component with a LIGHTAppTaskFrame component, which does the type of routing that a Mephisto task expects. This component is also responsible for requesting the correct authorization key for a given worker, and passing the synthesized URL to the underlying task component.
  • Updates the Task component to receive a passed URL and the submit function, though the latter is currently unused right now.
  • Creates the Mephisto run_task.py script that actually launches the mephisto job, requiring a preauth_secret that needs to be shared between the server launch and the Mephisto task. This allows Mephisto to sign the worker's auth token properly.

Some notes here, I've left a few messy components in the LIGHTAppTaskFrame file. These should probably be refactored out. Also unfortunately @JustinPinero I expect there to be a significant amount of manual churn merging your onboarding flow, considering some fairly substantial folder thrash required to get Mephisto up-and-running. When you merge onboarding into this, you'll have to add an onboarding_qualification to the local yaml configuration in order to see your onboarding view (after you insert it in the isOnboarding branch of LIGHTAppTaskFrame). See the local_test.yaml file in crowdsourcing/dialogues/multi_party_chat/hydra_confs folder for an example.

Testing

terminal 1>/Users/jju/LIGHT/deploy/web> ./deploy.sh local-no-models rebuild-task
terminal 2>/Users/jju/LIGHT/crowdsourcing/light_gameplay> python run_task.py 

Navigate to the desired location:
Screen Shot 2022-03-25 at 4 41 33 PM

JustinPinero and others added 30 commits April 23, 2021 19:19
JackUrb and others added 8 commits January 31, 2022 12:55
* Baseline code for standardizing data loading

* atomic teachers working

* Cleanup atomic

* quest goals teacher

* Predictive machines to projects

* Wild chat task and code and sweeps

* Updating sweep paths to new locations

* Cleaning up quests build

* Finished all _operational_ quests code

* Remaiing related sweeps

* Core quests rl project code

* Sweeps related to RL code

* Adding missing models

* Dropping sweeps

* Removing _almost_ all checkpoint links

* Uploading relevant models, adding download paths

* Core quests rl project code

* Sweeps related to RL code

* Adding missing models

* Removing _almost_ all checkpoint links

* Removing sweeps

* removing sweeps... again

* Build scripts now point to real file
* Creating example builder script for story agents project

* Writing play map example script
adding common sense + world builder code
* added gameplay task view

* added gameplay task view

* fixed message transmission in both gameapp reducer and task redux-slice

* removed boilerplate

* placed topmessage function at top of reducer
@JackUrb JackUrb requested a review from JustinPinero March 25, 2022 21:55
@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 Mar 25, 2022
Copy link
Copy Markdown
Contributor Author

@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.

Just two style nits on this final implementation (related to use of ternaries).

@JustinPinero only thing I think is missing is the change catching "you cant ...." messages to undo do actions.

setDoCounter(updatedWorkerDoCount);
}, [workerData])

return (
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Style nit: Large ternaries are visibly hard to process. As a rule of thumb, if the content of your ternary is larger than a line each for condition, option 1, and option 2, don't put it in a ternary and try to find another way to encode your logic.

The below could be:

if ((MINIMUM_NUMBER_OF_SAYS <= sayCounter) && (MINIMUM_NUMBER_OF_DOS <= doCounter)) {
    const modalContent = ...
} else {
    const modalContent = ...
}
If (url != null) {
    return ...
else {
    return <> null </>;
}

}
return (
<Form className="radioform-container">
{formQuestion
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note I'd consider this to be an OK example of a larger ternary, though you'd generally want to group the shorter context together to make this appropriate:

{ (formQuestion == null)
? null
: 
...
}

This helps constrain the context of the ternary such that it's not really deep in the code that you find the alternate branch.

@JackUrb JackUrb changed the base branch from main to attributes-backend April 12, 2022 19:52
@JackUrb JackUrb changed the base branch from attributes-backend to main April 12, 2022 19:53
JustinPinero and others added 4 commits April 19, 2022 11:59
* added assets

* added assets

* added PreviewView, added assets to preview view, began styling preview view

* added preview copy and screenshots, added instruction modal, added task copy from quip
@JackUrb JackUrb mentioned this pull request Apr 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants