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 tests for static_react_task example #795

Merged
merged 32 commits into from
Jul 8, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
b116336
✅ Added passing tests for static_react_task
Etesam913 Jun 16, 2022
a091438
🔥 Removed the starter test
Etesam913 Jun 16, 2022
379c9e9
🔧 Configured github actions for cypress
Etesam913 Jun 17, 2022
481433f
✏️ Added correct extension to workflow
Etesam913 Jun 17, 2022
8bcd15c
🔥 Removed extraneous code from cypress folder
Etesam913 Jun 17, 2022
ee4d978
📝 Added info on cypress testing to contributing.md
Etesam913 Jun 17, 2022
0aee860
🐛 Fixed request_agent test being inconsistent
Etesam913 Jun 18, 2022
7bc4f1b
✨ Added ability to link packages from command line
Etesam913 Jun 20, 2022
9ee7203
Merge branch 'main' of https://github.com/facebookresearch/Mephisto i…
Etesam913 Jun 20, 2022
ac59bbd
✨ Added command line arg to other examples
Etesam913 Jun 20, 2022
3c392a5
⛓ Updated github action to link to mephisto-task
Etesam913 Jun 20, 2022
113b8a8
✏️ Renamed the test
Etesam913 Jun 20, 2022
b944d78
🎨 Added spacing
Etesam913 Jun 20, 2022
a5cc65f
🔥 Delete commented out file
Etesam913 Jun 20, 2022
2e989e9
⏮ Reverting test back
Etesam913 Jun 20, 2022
f904089
🔥 Removed defineConfig from cypressConfig
Etesam913 Jun 21, 2022
721b4e6
🔀 Pulled from origin
Etesam913 Jun 21, 2022
0fc65a1
🐛 Fixed syntax bug
Etesam913 Jun 21, 2022
5702d9e
🔧 Updated hydra config to accept two new parameters
Etesam913 Jun 21, 2022
41cc544
📝 Added post_build_script to cypress start action
Etesam913 Jun 21, 2022
6b40eed
✏️ Updated contributing to two new sections
Etesam913 Jun 22, 2022
d5e846d
📦 Moved troubleshooting section
Etesam913 Jun 22, 2022
eb27dc9
✏️ Fixed typo in CONTRIBUTING.md
Etesam913 Jun 23, 2022
00e0023
✅ Simplified the tests for alerts
Etesam913 Jun 24, 2022
35b3f28
🔥 Removed extraneous code
Etesam913 Jun 24, 2022
d34a2ef
✏️ Added testing section to readme of task
Etesam913 Jun 26, 2022
e1393b9
✏️ Fixed typos in task_run fields
Etesam913 Jun 27, 2022
14beded
✏️ Renamed post_build_script field
Etesam913 Jun 27, 2022
db879d5
✏️ Replaced all occurrences of post_build_script
Etesam913 Jun 27, 2022
28c8431
🥅 Update build_bundle to work with no run_config
Etesam913 Jul 5, 2022
ff7ae27
🔥 Removed two properties from hydra_config
Etesam913 Jul 5, 2022
cd3a36c
🚨 Removed parameter warnings as they were too noisy
Etesam913 Jul 6, 2022
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
7 changes: 6 additions & 1 deletion examples/remote_procedure/mnist/run_task.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,12 @@ def handle_with_model(
)

task_dir = cfg.task_dir
build_custom_bundle(task_dir, cfg.mephisto)

build_custom_bundle(
task_dir,
force_rebuild=cfg.mephisto.task.force_rebuild,
post_install_script=cfg.mephisto.task.post_install_script,
)

operator.launch_task_run(cfg.mephisto, shared_state)
operator.wait_for_runs_then_shutdown(skip_input=True, log_rate=30)
Expand Down
6 changes: 5 additions & 1 deletion examples/remote_procedure/template/run_task.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,11 @@ def handle_with_model(
)

task_dir = cfg.task_dir
build_custom_bundle(task_dir, cfg.mephisto)
build_custom_bundle(
task_dir,
force_rebuild=cfg.mephisto.task.force_rebuild,
post_install_script=cfg.mephisto.task.post_install_script,
)
operator.launch_task_run(cfg.mephisto, shared_state)
operator.wait_for_runs_then_shutdown(skip_input=True, log_rate=30)

Expand Down
6 changes: 5 additions & 1 deletion examples/remote_procedure/toxicity_detection/run_task.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,11 @@ def handle_with_model(
)

task_dir = cfg.task_dir
build_custom_bundle(task_dir, cfg.mephisto)
build_custom_bundle(
task_dir,
force_rebuild=cfg.mephisto.task.force_rebuild,
post_install_script=cfg.mephisto.task.post_install_script,
Comment on lines +68 to +69
Copy link
Contributor Author

@Etesam913 Etesam913 Jul 5, 2022

Choose a reason for hiding this comment

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

An example of passing in the new parameters

)

operator.launch_task_run(cfg.mephisto, shared_state)
operator.wait_for_runs_then_shutdown(skip_input=True, log_rate=30)
Expand Down
7 changes: 6 additions & 1 deletion examples/static_react_task/run_task.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,12 @@ def onboarding_always_valid(onboarding_data):
)

task_dir = cfg.task_dir
build_custom_bundle(task_dir, cfg.mephisto)

build_custom_bundle(
task_dir,
force_rebuild=cfg.mephisto.task.force_rebuild,
post_install_script=cfg.mephisto.task.post_install_script,
)

operator.launch_task_run(cfg.mephisto, shared_state)
operator.wait_for_runs_then_shutdown(skip_input=True, log_rate=30)
Expand Down
29 changes: 24 additions & 5 deletions mephisto/tools/scripts.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
Utilities that are useful for Mephisto-related scripts.
"""

import warnings
from mephisto.abstractions.databases.local_database import LocalMephistoDB
from mephisto.operations.operator import Operator
from mephisto.abstractions.databases.local_singleton_database import MephistoSingletonDB
Expand All @@ -24,12 +25,14 @@
import hydra
import subprocess
from typing import (
Dict,
Tuple,
Any,
Type,
TypeVar,
Callable,
Optional,
Union,
cast,
TYPE_CHECKING,
)
Expand Down Expand Up @@ -221,7 +224,7 @@ def augment_config_from_db(script_cfg: DictConfig, db: "MephistoDB") -> DictConf
return script_cfg


def build_custom_bundle(custom_src_dir, run_config: DictConfig):
def build_custom_bundle(custom_src_dir, **kwargs: Union[str, bool]):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

kwargs is type Union[str, bool] because the parameter value can be a string(post_install_script) or bool(force_rebuild) currently.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't you be able to just write this as:

def build_custom_bundle(custom_src_dir, force_rebuild: Optional[bool] = None, post_install_script: Optional[str] = None)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I can do it like that, I was just following Pratik's suggestion here: #791 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

@Etesam913, you'd still be using kwargs with @JackUrb's suggestion above by naming the arguments in the list

more info: https://treyhunner.com/2018/04/keyword-arguments-in-python/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh so kwargs just refers to passing the arguments with keywords as opposed to using the **kwargs parameter. I got it 👍

"""Locate all of the custom files used for a custom build, create
a prebuild directory containing all of them, then build the
custom source.
Expand All @@ -234,8 +237,21 @@ def build_custom_bundle(custom_src_dir, run_config: DictConfig):

IGNORE_FOLDERS = {os.path.join(prebuild_path, f) for f in IGNORE_FOLDERS}
build_path = os.path.join(prebuild_path, "build", "bundle.js")

if "force_rebuild" not in kwargs:
warnings.warn(
"Consider adding the force_rebuild property to your hydra config and passing it into build_custom_bundle()\nSteps on how to do this: \nhttps://github.com/facebookresearch/Mephisto/issues/811",
stacklevel=2,
)

if "post_install_script" not in kwargs:
warnings.warn(
"Consider adding the post_install_script property to your hydra config and passing it into build_custom_bundle()\nSteps on how to do this: \nhttps://github.com/facebookresearch/Mephisto/issues/811",
stacklevel=2,
)
Copy link
Contributor Author

@Etesam913 Etesam913 Jul 5, 2022

Choose a reason for hiding this comment

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

New warning messages
They link to a closed issue that describes how to fix this


# see if we need to rebuild
Etesam913 marked this conversation as resolved.
Show resolved Hide resolved
if run_config.task.force_rebuild is False:
if "force_rebuild" not in kwargs or kwargs["force_rebuild"] == False:
if os.path.exists(build_path):
created_date = os.path.getmtime(build_path)
up_to_date = True
Expand Down Expand Up @@ -269,10 +285,13 @@ def build_custom_bundle(custom_src_dir, run_config: DictConfig):
)

if (
run_config.task.post_install_script is not None
and len(run_config.task.post_install_script) > 0
"post_install_script" in kwargs
and kwargs["post_install_script"] is not None
and isinstance(kwargs["post_install_script"], str)
):
subprocess.call(["bash", run_config.task.post_install_script])
post_install_string: str = str(kwargs["post_install_script"])
if len(post_install_string) > 0:
subprocess.call(["bash", post_install_string])

webpack_complete = subprocess.call(["npm", "run", "dev"])
if webpack_complete != 0:
Expand Down