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

Updating mephisto-task to use the Architect 1.0 API, pushing to static tasks. #660

Merged
merged 10 commits into from
Feb 17, 2022

Conversation

JackUrb
Copy link
Contributor

@JackUrb JackUrb commented Feb 12, 2022

Overview

As part of #655, this PR contains the full set of changes to update mephisto-task to work with the 1.0 API for architects and packets. Upgrades mephisto-task to version 2.0.0 to note this. Also happens to fix the bug in #419 while we're at it. Incorporates the changes from #659 to allow me to do the editing.

Implementation

There are a lot of changes here, but the most critical surround the change to the API for mephisto-task's AgentState. We used to directly keep track of a number of variables that would persist in our local storage, when these were frequently task-specific. By allowing these to be parsed from 'live data' packets instead, we can allow the downstream tasks to decide what to keep and how to parse, rather than leaking that through the abstraction.

One element that remains a bit leaky - triggering these kinds of state callbacks requires that the live data packet you send contains a { 'state': {...}} field. I've noted this in the architecture doc. Ultimately I'm fine with having the packets serve two functions starting at the mephisto-task level, as I think it lets me abstract the Packets from the data that is sent with them. It may be worthwhile to add some kind of function to the Agent class like send_state or something that just acts as a wrapper around this send, but that could be unnecessary.

With this change, the callbacks that trigger for useMephistoTask and useMephistoLiveTask needed to be updated somewhat, but hopefully in a way that's clear with this context.

For a full list of changes:

  • useMephistoTask
    • Sets blockedExplanation as part of the state, this way we can pull from an AgentDetails event when the Mephisto backend provides a reason for us to show the worker.
    • Updates both the regular registration handshake as well as the post-onboarding handshake to use the new one-step semantics
    • Deletes unused code from utils.js
  • useMephistoLiveTask:
    • Updates the socket_handler.jsx to use the new packet types
    • Removes onStateUpdate as we no longer track this at the packet level
    • Fixes [Clarifications] Testing in a local environment #419 by inheriting the location's protocol instead of the localhost hack
    • Creates an effect for receiving initial data that checks raw_messages to allow a mephisto-task task to replay them, restoring the original state and messages present.
    • Implements the state switch noted above for onMessageReceived, which is now pulled out for use with raw_messages.
  • Removes getWorkerRegistrationInfo from the required functions of wrap_crowd_source.js as we only have a single step registration handshake now. Updates the documentation to reflect this.
  • Fixes how StaticAgentState handles parsing submissions with files. Now that we directly pass the submitted data rather than the full context packet, we don't need to extract out from onboarding or task data. We can then check for files and do final file parsing.
  • Minor bugfixes on the StaticHTMLBlueprint tasks. Transitioning from onboarding now works correctly in a way that required a refresh to fix before.
  • Updates packages throughout to be using the newer versions of webpack and webpack-cli
  • Small fixes to hydra config .yamls that were actually broken :(

Testing

Automated tests should still be passing, considering this isn't touched.
I updated both of the StaticBlueprint task examples, and tried out various configuration steps. Went fully from onboarding through to completing a task in both setups.
React preview
Screen Shot 2022-02-10 at 4 52 06 PM
React task
Screen Shot 2022-02-11 at 9 35 01 AM
React onboarding
Screen Shot 2022-02-11 at 9 54 40 AM
HTML task (with file upload)
Screen Shot 2022-02-11 at 10 45 17 AM
HTML onboarding
Screen Shot 2022-02-11 at 10 44 20 AM

@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 Feb 12, 2022
@JackUrb JackUrb mentioned this pull request Feb 12, 2022
16 tasks
Copy link
Contributor

@pringshia pringshia left a comment

Choose a reason for hiding this comment

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

Wow, I'm loving these changes and the reduction in complexity incurred by them. Overall I think it will make Mephisto easier to maintain, more versatile, and friendlier to newcomers. Appreciate all of the fixes also rolled in here.

Left a few comments throughout.

Also thanks for the thorough PR description and outline. It made reviewing this PR significantly easier.

packages/mephisto-task/src/live.js Outdated Show resolved Hide resolved
packages/mephisto-task/src/live.js Show resolved Hide resolved
packages/mephisto-task/src/live.js Outdated Show resolved Hide resolved
packages/mephisto-task/README.md Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Feb 15, 2022

Codecov Report

Merging #660 (3eb3f35) into node-router-update (0a44f70) will decrease coverage by 0.10%.
The diff coverage is 0.00%.

Impacted file tree graph

@@                  Coverage Diff                   @@
##           node-router-update     #660      +/-   ##
======================================================
- Coverage               64.46%   64.35%   -0.11%     
======================================================
  Files                      91       91              
  Lines                    8492     8489       -3     
======================================================
- Hits                     5474     5463      -11     
- Misses                   3018     3026       +8     
Impacted Files Coverage Δ
...eprints/abstract/static_task/static_agent_state.py 32.83% <0.00%> (+1.40%) ⬆️
...tractions/architects/channels/websocket_channel.py 69.67% <0.00%> (-9.02%) ⬇️

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 0a44f70...3eb3f35. Read the comment docs.

Base automatically changed from node-router-update to architect-api February 17, 2022 17:41
@JackUrb JackUrb merged commit a7f5683 into architect-api Feb 17, 2022
@JackUrb JackUrb deleted the mephisto-task-update branch February 17, 2022 17:43
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants