Enable symlinking js bundle via Hydra prop to make for easier local development for static_react_task

[pringshia]

Overview

@ytsheng mentioned that it's annoying that python run_task.py requires a rerun every time a React code change is made. I agreed.

This PR update introduces a Hydra config property called mephisto.blueprint.link_task_source which can be set to true, to remediate this.`

From the accompanying README update in this PR:

Local Development

For local development, you may want to have changes made to your React code reflect locally without having to restart the server each time. To enable this, update the Hydra config's mephisto.blueprint.link_task_source value to true (default is false).

After running python run_task.py, you can then run npm run dev:watch in the webapp folder to auto-regenerate the task_source file. Since the task source file is now symlinked, simply refreshing the browser will show changes. You won't need to kill and restart the server each time anymore.

Testing

1. Update /examples/static_react_test/hydra_configs/conf/example.yaml to include link_task_source: true:

2. Run python run_task.py
3. Run npm run dev:watch from the /examples/static_react_test/webapp folder in a separate console

• if you don't have this script in your package.json file, you can add something like "dev:watch": "webpack --mode development --watch" under the scripts key.

5. Load the page at http://localhost:3000
6. Edit something in /examples/static_react_task/webapp/src/app.jsx for example, in the isPreview code block where the task description is written
7. Reload the page at http://localhost:3000 without restarting the python server
8. Ensure that the change is reflected

[codecov-commenter]

Codecov Report

Merging #509 (f79c7ef) into master (6d3ad9f) will decrease coverage by 0.06%.
The diff coverage is 20.00%.

@@            Coverage Diff             @@
##           master     #509      +/-   ##
==========================================
- Coverage   65.72%   65.66%   -0.07%     
==========================================
  Files          79       79              
  Lines        7239     7246       +7     
==========================================
  Hits         4758     4758              
- Misses       2481     2488       +7     

Impacted Files	Coverage Δ
...lueprints/abstract/static_task/static_blueprint.py	41.00% <ø> (-0.59%)	⬇️
...nts/static_react_task/static_react_task_builder.py	38.09% <0.00%> (-6.35%)	⬇️
...prints/static_react_task/static_react_blueprint.py	60.46% <20.00%> (-5.33%)	⬇️
...ephisto/abstractions/architects/local_architect.py	67.61% <100.00%> (ø)

---

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 6d3ad9f...f79c7ef. Read the comment docs.

[pringshia]

Unrelated to the PR scope, I noticed that this is a duplicate field, it's already defined higher up. Cleaning up.

[hackgoofer]

Confirmed that it works on my end. Thank you!!!

[JackUrb]

I feel we should do a bit more to prevent accidental mismatch between input configuration arguments. If someone tries to launch externally with this option set, the behavior may not be as expected. For this, you can add a case to StaticReactBlueprint.assert_task_args that throws if launching with cfg.architect._architect_type == heroku and this option set to true.

In the longer run, I think we'll need to dig deeper on compatibility between the different interfaces, but I don't want to block this implementation for that discussion.

[JackUrb]

Approved pending the compatibility changes.

[pringshia]

Makes sense @JackUrb - I was thinking the same thing, as this config option only really makes sense for when architect=local and for blueprint=static_react_task, even though it lives within the blueprint config.

I wasn't sure whether to silently ignore the irrelevant config when architect != local or whether to fail. I don't have a strong opinion either way, happy to add in the check in assert_task_args.

I think it may make more sense to have it be cfg.architect._architect_type != local vs cfg.architect._architect_type == heroku. any thoughts on that? ofc right now we only have 2 but this will make it so we dont have to change it again in the future if we add something like ec2

[JackUrb]

I was wondering if there would ever be a situation where we want to use _architect_type == mock for this implementation with the above suggestion. Indeed we only have a few right now, perhaps the safer option is to gate to local for now.

[pringshia]

True, mock could be a candidate in the future. for now, I'll assert that it only works for local (i prefer explicit allowlists over inexplicit blocklists), and in the future as we test and integrate mock we can add that to the allowlist as well

[pringshia]

Added task args validation to ensure link_task_source is not accidentally specified with something like heroku

[JackUrb]

Thanks for adding the check!