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

Fixing parlai onboarding #439

Merged
merged 12 commits into from
Apr 23, 2021
Merged

Fixing parlai onboarding #439

merged 12 commits into from
Apr 23, 2021

Conversation

JackUrb
Copy link
Contributor

@JackUrb JackUrb commented Apr 19, 2021

Overview

#421 included a bug for specific cases of ParlAIBlueprint onboarding. This fix resolves the issue, and also resolves an issue where the timeout status wasn't being pushed forward in onboarding.

Details

#421 introduced a bug specifically for ParlAIBlueprint onboarding worlds, as the ParlAIBlueprint is the only type of live task wherein either the backend (the World) or the frontend (via handleSubmit) can submit the onboarding. In the case that the world exited cleanly, this would lead to a message to the frontend telling it to submit.

Unfortunately, #421 improved the onboarding flow to ensure that the transition from running onboarding to registering would happen in the same order every time. Specifically, the requirement is that _on_submit_onboarding now has to trigger the termination of the onboarding runner. In the case where the backend triggered this for parlai onboarding by just exiting an onboarding world, that wasn't the order things happened in.

This updates the ParlAChatITaskRunner to first mark the agent as done, then wait for the submit packet (to come through _on_submit_onboarding before terminating.

Testing

Launched a task world with onboarding worlds that submit on the backend, was able to transition into the main world with no refreshes, no disconnects, and no other behavior.
Launched a task world with onboarding, timed out, then ensured the timeout was pushed to the frontend.

@JackUrb JackUrb requested a review from mojtaba-komeili April 19, 2021 20:31
@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 Apr 19, 2021
done_act = agent.act()
if done_act is not None:
break
time.sleep(0.3)
Copy link
Contributor

Choose a reason for hiding this comment

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

question: could we have this as a variable in case we wanted to find the optimal value?

@mojtaba-komeili
Copy link
Contributor

The code looks good to me. I tried this in mock, sandbox and live. Mock worked well. In sandbox, when it was transitioning it looked like it was refreshing a bit too long, but it was generally good. The live (prod) version is going fine but a bit too slow (that might be related to other factors though).

@JackUrb JackUrb force-pushed the mturk-hit-sync-speedup branch from c74f1e9 to d222f4d Compare April 20, 2021 18:04
@JackUrb JackUrb force-pushed the fixing-parlai-onboarding branch from d0a6231 to 352aa28 Compare April 20, 2021 18:06
@codecov-commenter
Copy link

Codecov Report

Merging #439 (352aa28) into mturk-hit-sync-speedup (d222f4d) will increase coverage by 0.11%.
The diff coverage is 46.15%.

Impacted file tree graph

@@                    Coverage Diff                     @@
##           mturk-hit-sync-speedup     #439      +/-   ##
==========================================================
+ Coverage                   65.95%   66.07%   +0.11%     
==========================================================
  Files                          78       78              
  Lines                        7084     7097      +13     
==========================================================
+ Hits                         4672     4689      +17     
+ Misses                       2412     2408       -4     
Impacted Files Coverage Δ
.../blueprints/parlai_chat/parlai_chat_task_runner.py 28.00% <14.28%> (-0.68%) ⬇️
mephisto/operations/supervisor.py 79.48% <83.33%> (+1.47%) ⬆️
...tractions/architects/channels/websocket_channel.py 80.23% <0.00%> (+1.16%) ⬆️
mephisto/abstractions/architects/mock_architect.py 91.50% <0.00%> (+2.61%) ⬆️

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 d222f4d...352aa28. Read the comment docs.

Base automatically changed from mturk-hit-sync-speedup to master April 23, 2021 20:25
@JackUrb JackUrb changed the base branch from master to update-internal-inits April 23, 2021 20:26
@JackUrb JackUrb changed the base branch from update-internal-inits to master April 23, 2021 20:26
@JackUrb JackUrb merged commit e01d0ff into master Apr 23, 2021
@JackUrb JackUrb deleted the fixing-parlai-onboarding branch April 23, 2021 20:34
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