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

[TODO Roundup] Make Operator more configurable #94

Closed
5 tasks
JackUrb opened this issue Apr 23, 2020 · 2 comments
Closed
5 tasks

[TODO Roundup] Make Operator more configurable #94

JackUrb opened this issue Apr 23, 2020 · 2 comments

Comments

@JackUrb
Copy link
Contributor

JackUrb commented Apr 23, 2020

Summary

The Mephisto Operator is the most powerful class in all of Mephisto, in that it does the heavy lifting of launching and running jobs, and configuring the rest of the Mephisto architecture to do what's intended. One painful area of this flow is actually configuring the launch itself.

For context, Mephisto needs to be able to send configuration options forward to the frontend in order for people to be able to launch jobs from there. These configuration options also need to be persisted somehow to ensure that equivalent jobs can be re-launched later. As such there are a number of flows around trying to send required arguments. See Operator.parse_and_launch_run for the full flow that finds and parses these arguments.

The goal is to simplify the process for launching scripts from argument dicts, and to ensure the contents within are saved to the task_run's data directory for reproducibility in the future. (As of now, the TaskConfig handles part of this, but only does so for the parsed string arguments and not those passed additionally by dict).

As a stretch goal, it would be nice to move away from ArgParser dependence, as this has us locked to a few less-than-optimal coding situations (including converting passed dictionaries to string args, re-parsing, initializing an argparser during a critical path, ++).

Requirements

  • Create parse_arguments_and_launch_run and launch_run_from_argument_dict methods in Operator, which act as wrappers around the basic parse_and_launch_run function. This refactor will involve sharing some code between the two methods (with the former following essentially the same code flow as parse_and_launch_run has now). The big question to solve is how to properly handle ensuring both methods pass through the same assert_args flow when the version from the dict doesn't run through the argparser.
  • Update examples to use the launch_run_from_argument_dict method.
  • Add and wire missing arguments.
  • Write documentation on parse_arguments_and_launch_run.

Wishlist

  • Create a new class (potentially extending ArgumentParser) that handles the process of registering arguments, being able to display registered arguments, showing default values, etc. See argparse_parser.py for our existing solution.
@pringshia
Copy link
Contributor

I suspect some of the subtasks in here may already be completed?

@JackUrb
Copy link
Contributor Author

JackUrb commented Jan 27, 2021

Ah this was actually fully resolved by #246

@JackUrb JackUrb closed this as completed Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants