-
Notifications
You must be signed in to change notification settings - Fork 77
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
Add Screening Example #883
Conversation
Codecov Report
@@ Coverage Diff @@
## main #883 +/- ##
==========================================
+ Coverage 64.61% 64.64% +0.02%
==========================================
Files 108 108
Lines 9313 9312 -1
==========================================
+ Hits 6018 6020 +2
+ Misses 3295 3292 -3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
🎨 Improved button styling on onboarding page
What are your thoughts on putting a video in a documentation page? I compressed down the video, so it is about 9.4mb in size right now. I do think that it would help a newcomer quickly understand how screening units work. |
I updated the video |
@@ -25,6 +25,7 @@ | |||
"prism-react-renderer": "^1.2.1", | |||
"react": "^17.0.1", | |||
"react-dom": "^17.0.1", | |||
"react-player": "^2.10.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for being able to play videos in mdx file.
is_using_screening_units = cfg.mephisto.blueprint["use_screening_task"] | ||
shared_state = None | ||
if not is_using_screening_units: | ||
shared_state = SharedStaticTaskState( | ||
static_task_data=[ | ||
{"text": "This text is good text!"}, | ||
{"text": "This text is bad text!"}, | ||
], | ||
validate_onboarding=onboarding_always_valid, | ||
) | ||
|
||
else: | ||
""" | ||
When using screening units there has to be a | ||
few more properties set on shared_state | ||
""" | ||
shared_state = SharedStaticTaskState( | ||
static_task_data=[ | ||
{"text": "This text is good text!"}, | ||
{"text": "This text is bad text!"}, | ||
], | ||
validate_onboarding=onboarding_always_valid, | ||
on_unit_submitted=ScreenTaskRequired.create_validation_function( | ||
cfg.mephisto, | ||
validate_screening_unit, | ||
), | ||
screening_data_factory=my_screening_unit_generator(), | ||
) | ||
shared_state.qualifications += ScreenTaskRequired.get_mixin_qualifications( | ||
cfg.mephisto, shared_state | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are using the screening unit conf they are more properties that you need to add to your SharedStaticTaskState
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add these dynamically to the SharedStaticTaskState
after the case:
share_state = ...
if is_using_screening_units:
shared_state.on_unit_submitted = ...
...
To reduce duplication a bit
<div | ||
style={{ | ||
width: "100%", | ||
padding: "1.5rem 0", | ||
display: "flex", | ||
justifyContent: "center", | ||
}} | ||
> | ||
Move to main task. | ||
</button> | ||
<button | ||
className="button is-link" | ||
onClick={() => onSubmit({ success: true })} | ||
> | ||
Move to Main Task | ||
</button> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was peeved by the fact that the onboarding button was not centered on the page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeeeeaaah this could've been adjusted long ago hahaha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the new docs page and examples! Only a tiny nit on code duplication, but otherwise I'm excited to have solid examples of this workflow now, and for it to be wrapped into the two most commonly used blueprints.
@@ -8,6 +8,7 @@ | |||
import csv | |||
from genericpath import exists | |||
import os | |||
from rich import print |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unneeded import
@@ -470,6 +470,7 @@ async def register_agent( | |||
function="get_valid_units_for_worker" | |||
).time(): | |||
units = task_run.get_valid_units_for_worker(worker) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Odd whitespace changes in this file
Curious to hear more about this, but thanks for fixing! |
There were some bugs with hydra conf variables not existing. The changes made to |
This was my bad - I had made a mistake in the Mephisto magic that extracts the The issue: I wanted to be able to only take the terminal class of a
The fix was to do a step 2.5 that drops any subclasses of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty nice! Thanks for improving the screening developer experience. It's an often-used feature of Mephisto and helps greatly in assuring data quality.
|
||
|
||
@task_script(default_config_file="launch_with_local") | ||
def main(operator: Operator, cfg: DictConfig) -> None: | ||
tasks: List[Dict[str, Any]] = [{}] * cfg.num_tasks | ||
tasks: List[Dict[str, Any]] = [{"isScreeningUnit": False}] * cfg.num_tasks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could also be reverted back to what it was (no property aka undefined
) and the frontend would still work since it's doing a truthy boolean check, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's being used like this: const NUM_ANNOTATIONS = taskData.isScreeningUnit ? 1 : 3;
) | ||
shared_state.qualifications += ScreenTaskRequired.get_mixin_qualifications( | ||
cfg.mephisto, shared_state | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is unrelated to this PR, but it seems that ScreenTaskRequired.get_mixin_qualifications
is not even using shared_state
despite requiring it in the signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_mixin_requirements
has a shared signature for all BlueprintMixin
s, as ideally this will be replaced with <>Blueprint.get_mixin_qualifications
automatically pulling required qualifications into the state under the hood.
shared_state = SharedRemoteProcedureTaskState( | ||
static_task_data=tasks, | ||
function_registry=function_registry, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could also be condensed based on Jack's earlier comment
Overview
This pr configures the option to run screening units on
static_react_task
.screening_example.yaml
configurationSharedStaticTaskState
Mnist Video (this is the showcase in the documentation page below)
mnist-screening-example.mp4
Static React Task Video
screening_demo_example.mp4
New Documentation Page for Screening Unit