Removed empty key/value in validate_onboarding #4492
Conversation
Removed `submitted_data` in `validate_onboarding`, as `data` was not in `status_message` and caused an empty value in the `final_status`
|
Hi @evelynkyl! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
12 similar comments
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
@EricMichaelSmith I'm surprised that per-turn eval was passing here before - does the current test not include onboarding? Something we'll be looking at more as we develop an E2E testing framework for Mephisto tasks. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
There was a problem hiding this comment.
Yeah, I think this fix is right, thanks @evelynkyl for adding - if you look at https://github.com/facebookresearch/ParlAI/pull/4426/files, this same fix was done for model_chat as part of the Mephisto 1.0 upgrade. However, it looks like a bunch of CI checks are failing now, for reasons that seem to be unrelated to this PR - can you maybe try doing a git merge from the main branch on facebookresearch to see if the breakages are just related to not having the most recent commits?
@JackUrb I had been afraid of this - unfortunately we haven't had time to put a unit test on per-turn eval yet. I'll ask around to see if anyone else has bandwidth for this currently
|
@EricMichaelSmith Feel free to defer for now - Mephisto shouldn't have any breaking changes prior to the E2E integration testing PR, so work fixing now would likely just be easier after that framework is provided. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
7 similar comments
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
This PR has not had activity in 30 days. Closing due to staleness. |
|
@EricMichaelSmith We should probably approve and merge this! Failing tests here seem unrelated to these changes. |
@JackUrb Yeah fair enough - I just created a new PR for this at #4560 and tested that it fixes the issue. Closing this PR given that new one |
I removed
submitted_datainvalidate_onboarding, asdatawas not instatus_messageand caused an empty value in thefinal_statusPatch description
In per_turn_eval's
worlds.py, the functionvalidate_onboardingwas trying to get the variabledatain the dict ofstatus_message.Butdatadoes not exist in thestatus_messageand it is causingfinal_statusto always be None, therefore blocking all workers.Here's the issue facebookresearch/Mephisto#748 where I wanted to reset onboarding qualification, I used a different worker id but found out
validate_onboardingended up keep blocking me because thefinal_statusis always None.Testing steps
Launch
run.pyand do onboarding. Perform onboarding correctly and incorrectly to test qualified and disqualified workers and check ifdataever exists instatus_messagefor this task.