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

ParlAI Assignment Fixes pt 2 #878

Merged
merged 3 commits into from
Aug 2, 2022
Merged

ParlAI Assignment Fixes pt 2 #878

merged 3 commits into from
Aug 2, 2022

Conversation

JackUrb
Copy link
Contributor

@JackUrb JackUrb commented Aug 1, 2022

Following #875, there's now the condition where too many assignments are being launched. While usually harmless to have this redundancy, it introduced a new race conditions where two threads could be launched for a single assignment. Having two assignment threads for the same assignment, being not ideal, is now patched.

Also bumps hydra in-line with the new expectations as of #869.

@JackUrb JackUrb requested a review from pringshia August 1, 2022 20:27
@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 Aug 1, 2022
@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2022

Codecov Report

Merging #878 (1a293af) into main (f1b6766) will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #878      +/-   ##
==========================================
+ Coverage   64.45%   64.49%   +0.04%     
==========================================
  Files         108      108              
  Lines        9311     9312       +1     
==========================================
+ Hits         6001     6006       +5     
+ Misses       3310     3306       -4     
Impacted Files Coverage Δ
.../blueprints/parlai_chat/parlai_chat_agent_state.py 29.03% <ø> (+0.46%) ⬆️
mephisto/utils/metrics.py 29.10% <0.00%> (-0.22%) ⬇️
...ractions/blueprints/mixins/screen_task_required.py 74.74% <0.00%> (+0.25%) ⬆️
mephisto/abstractions/architects/mock_architect.py 90.84% <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 f1b6766...1a293af. Read the comment docs.

@JackUrb JackUrb changed the title Bump hydra to 1.2 ParlAI Assignment Fixes pt 2 Aug 1, 2022
@JackUrb JackUrb requested a review from chiehminwei August 1, 2022 21:16
return # need to wait for all agents to be here to launch

for queried_agent in agents:
if queried_agent.get_status() != AgentState.STATUS_WAITING:
Copy link
Contributor

Choose a reason for hiding this comment

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

Under what condition would this not be true? When things haven't synced to the worker threads?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When two workers connect at the same time, and both have had an _assign_unit_to_agent thread called, and have thus both had Agents created, but only one of those threads has made it to the update_status call.

@JackUrb JackUrb merged commit bd4c80b into main Aug 2, 2022
@JackUrb JackUrb deleted the bump-hydra branch August 2, 2022 23:04
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