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

Add info on link_task_source to docs #885

Merged
merged 6 commits into from
Aug 10, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions docs/web/docs/guides/tutorials/custom_react.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,30 @@ mephisto:
task_tags: "test,simple,button"
```

The only change we'll make is to the `task_name` (as you should on any new tasks!), but we will update the other fields as we go. It's important to note the `task_source` and `extra_source_dir` arguments though, as this is where the `StaticReactBlueprint` class will be looking for the compiled React app's Javascript bundle as well as a folder for extra static resources for the page, respectively.
It is important to give a new `task_name` as we are creating a custom task. For local development **only** it also makes sense to set `link_task_source` to true. This allows changes to propagate to your localhost server when you reload the page (otherwise you would have to shutdown and restart the server to see changes).

The `task_source` and `extra_source_dir` arguments are also of importance, as this is where the `StaticReactBlueprint` class will be looking for the compiled React app's Javascript bundle as well as a folder for extra static resources for the page, respectively.

### 1.2 Launching the task
From the current directory, you should be able to execute the run script and get a job working. We're using a different `task_name` to prevent polluting our later task with data that won't share the same format. It is a good practice to do this with initial iterations, and to change the `task_name` any time you change input or output arguments.

You can update the `task_name` and `link_task_source` values in your config and run the task like below
```bash
python run_task.py mephisto.task.task_name=custom-react-tutorial-iterating
python run_task.py
```

or you can set them when you run the task:

```bash
python run_task.py mephisto.task.task_name=custom-react-tutorial-iterating mephisto.blueprint.link_task_source=true
```
This will launch a simple task where an annotator is supposed to note a sentence as being good or bad. Clicking a button auto-submits the task. In the next sections we'll add other content.

To establish a link where your changes will be propagated to the localhost server(when you reload), create a separate terminal window and run
```bash
cd webapp && npm run dev:watch
```

Moving forward, we'll update this task so that workers are able to edit the text as well as rate the original sentence.

## 2. Providing new data to the task
Expand Down
2 changes: 1 addition & 1 deletion mephisto/operations/operator.py
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ def launch_task_run_or_die(
raise e

live_run.task_launcher.create_assignments()
live_run.task_launcher.launch_units(task_url)
live_run.task_launcher.launch_units(url=task_url, run_config=run_config)

self._task_runs_tracked[task_run.db_id] = live_run
task_run.update_completion_progress(status=False)
Expand Down
17 changes: 13 additions & 4 deletions mephisto/operations/task_launcher.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
if TYPE_CHECKING:
from mephisto.data_model.task_run import TaskRun
from mephisto.abstractions.database import MephistoDB

from omegaconf import DictConfig, OmegaConf
import threading
from mephisto.utils.logger_core import get_logger
import types
Expand Down Expand Up @@ -180,7 +180,7 @@ def generate_units(self):
if not self.unlaunched_units:
break

def _launch_limited_units(self, url: str) -> None:
def _launch_limited_units(self, url: str, run_config: Optional[DictConfig]) -> None:
"""use units' generator to launch limited number of units according to (max_num_concurrent_units)"""
# Continue launching if we haven't pulled the plug, so long as there are currently
# units to launch, or more may come in the future.
Expand All @@ -191,15 +191,24 @@ def _launch_limited_units(self, url: str) -> None:
if unit is None:
break
unit.launch(url)
if (
run_config is not None
and run_config.blueprint.link_task_source == False
):
logger.info(
"If you want your server to update on reload whenever you make changes to your webapp, then make sure to set \n\nlink_task_source: true\n\nin your task's hydra configuration and run \n\ncd webapp && npm run dev:watch\n\nin a separate terminal window. For more information check out:\nhttps://mephisto.ai/docs/guides/tutorials/custom_react/#12-launching-the-task"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the rationale for putting the checking code here as opposed to somewhere else? Not challenging, just curious. Is there any other place this could be placed up the function call stack?

I find that adding a new param to the function just for this functionality to be a bit of a code smell, though I'm willing to be talked out of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to be messing up the python build test so I will probably have to find a new place to put it.

I put it there so that I could get access to the run_config variables. I need to check if link_task is set to false before showing anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

e.g. why not put it in launch_task_run_or_die?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto to @pringshia's concern here. While I'm a big fan of raising visibility on this, I think the best place to put this warning is actually inside of the assert_task_args method for the StaticReactBlueprint. (If the architect is in the allowed architects and link_task_source is false, let people know).

Copy link
Contributor Author

@Etesam913 Etesam913 Aug 10, 2022

Choose a reason for hiding this comment

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

Yeah, this seems like a way better solution. This will also most likely prevent the tests from failing.

if self.generator_type == GeneratorType.NONE:
break
self.finished_generators = True

def launch_units(self, url: str) -> None:
def launch_units(self, url: str, run_config: Optional[DictConfig]) -> None:
"""launch any units registered by this TaskLauncher"""
self.launch_url = url
self.units_thread = threading.Thread(
target=self._launch_limited_units, args=(url,), name="unit-generator"
target=self._launch_limited_units,
args=(url, run_config),
name="unit-generator",
)
self.units_thread.start()

Expand Down