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 example for scripting with cmd interface #1321

Merged
merged 6 commits into from
Sep 9, 2022
Merged

Conversation

lshamis
Copy link
Contributor

@lshamis lshamis commented Aug 2, 2022

Description

This PR adds an example of using the cmd interface to script complex launch ordering and such.

This PR also adds a --wait flag to the up command, blocking it until process has completed.

Please check the options that are relevant.

  • Bug fix (non-breaking change that fixes an issue)
  • Proposes a change (non-breaking change that isn't necessarily a bug)
  • Refactor
  • New feature (non-breaking change that adds a new functionality)
  • Breaking change (fix or feature that would break some existing functionality downstream)
  • This is a unit test
  • Documentation only change
  • Datasets Release
  • Models Release

Type of requested review

  • I want a thorough review of the implementation.
  • I want a high level review.
  • I want a deep design review.

Before and After

If this PR introduces a change or fixes a bug, please add before and after screenshots (if this is causes a UX change) or before and after examples(this could be plain text, videos etc ) to demonstrate the change.

If this PR introduces a new feature, please also add examples or screenshots demonstrating the feature.

Testing

Please describe the tests that you ran to verify your changes and how we can reproduce.

Checklist:

  • I have performed manual end-to-end testing of the feature in my environment.
  • I have added Docstrings and comments to the code.
  • I have made changes to existing documentation where needed.
  • I have added tests that show that the PR is functional.
  • New and existing unit tests pass locally with my changes.
  • I have added relevant collaborators to review the PR before merge.
  • [Polymetis only] I ran on hardware (1) all scripts in tests/scripts, (2) asv benchmarks.

@lshamis lshamis requested a review from zephirefaith August 2, 2022 22:41
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 2, 2022
Copy link
Contributor

@zephirefaith zephirefaith left a comment

Choose a reason for hiding this comment

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

I am unsure about the usage of wait still, and would like to understand how customisable it is. Examples are the best documentations available to all user so if we can flesh out the limitations, capabilities of the wait flag here, I think that'd be very useful. My questions are in the comments. I'd like to extend this example with launch_1.py (e.g.) with some documentation to explain those behaviours.

time.sleep(0.5)

mrp.cmd.up("set_foo", wait=True)
mrp.cmd.up("get_foo", attach=True, wait=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this line mean that process being up'd will wait for every process defined before it to finish?

If yes, in order to only depend on a subset of processes defined before would the right call be:

list_of_wait_procs = ["my_other_process", "their_process"]
mrp.cmd.up("my_new_process", attach=True, wait=True, procs=list_of_wait_procs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the wait here applies to the get_foo being run.

We don't want to down the redis server until get_foo completed.

The wait=True on set_foo was to ensure set_foo completed before we run get_foo

@@ -0,0 +1,7 @@
# Example 16: Scripting msetup commands

All `mrp` commands are available in both CLI and script form.
Copy link
Contributor

Choose a reason for hiding this comment

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

mrp up proc_name --wait other_proc should make proc_name's start conditional on exit of other_proc. Earlier mrp had a standalone wait argument as well, I am not sure what is the difference between the 1st invokation and mrp wait usage. If there is not a difference I think the up with --wait flag makes a lot more sense and is consistent with how mrp is used. If not, could you please explain the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mrp wait <procs> waits for the listed processes to complete.
mrp up <procs> --wait runs the listed processes and waits for them to complete.

--wait is a boolean flag. It doesn't take arguments and isn't ordered.

The following are identical:

1)
mrp up proc_name --wait other_proc

2)
mrp up proc_name other_proc --wait

3)
mrp up --wait proc_name other_proc

4)
mrp up proc_name other_proc
mrp wait proc_name other_proc

In all three cases, proc_name and other_proc are run and the terminal hangs until the two processes complete.

@zephirefaith zephirefaith merged commit 3308290 into main Sep 9, 2022
@zephirefaith zephirefaith deleted the lshamis/mrp_script branch September 9, 2022 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants