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

Baseline simulations and allPhysics configuration #176

Merged
merged 32 commits into from
Mar 19, 2020

Conversation

nbren12
Copy link
Contributor

@nbren12 nbren12 commented Mar 7, 2020

Introduces some changes to allow running baseline prognostic simulations with ML included.

Update by @brianhenn 2020-Mar-15: I've added a bunch of edits to the workflow orchestrator to redesign its syntax and argument handling to be more generalizable and enhance the orchestrator maintainability. This includes:

  • general handling of arguments for both inputs and other types of arguments, with ability to specify from/location, optional arguments, and the ability to pass multiple argument values available for all arguments. This resolves Making the orchestrator work with arbitrary CLIs #174 .
  • changed the argument passing syntax to reflect the merging of inputs/extra arguments; arguments are now passed in *positional_args + output_location + **{optional_arg: arg_value} order.
  • updated all of the current workflow step submission scripts to work with the new syntax
  • removed all of the intermediate orchestrator_submit.sh scripts as they are no longer needed by the orchestrator (all orchestrator calls are now directly to python), while maintaining any single-step shell script functionality in individual steps.
  • added the ability to launch Dataflow jobs from the orchestrator without an intermediate script using the --runner argument

These changes are included in this PR because they (among many other things) allow for the prognostic run to be called with the ML model path argument as an optional input from the model training step, to support running the ML and baseline prognostic runs together under the orchestrator.

@nbren12 nbren12 marked this pull request as ready for review March 10, 2020 16:20
@brianhenn brianhenn requested a review from frodre March 16, 2020 16:22
@brianhenn
Copy link
Contributor

Copy link
Contributor

@frodre frodre left a comment

Choose a reason for hiding this comment

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

Thanks, @brianhenn for this major update to orchestrator functionality! Doing away with the intermediate scripts really cleans things up. I suspect that not too many people are still using the original submit_job.sh scripts, so perhaps we could put some deprecation warning in those and eventually phase them out too. Thanks for being thorough with the updates to the READMEs for individual steps and the end-to-end workflow.

I have a few suggestions and requested changes, mostly in get_experiment_steps_and_args.py, that I'd like to see addressed before merging. Cheers!

Edit: Also, thanks, @nbren12!

Copy link
Contributor

@frodre frodre left a comment

Choose a reason for hiding this comment

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

Thanks, @brianhenn for the changes. Please add a unique tag on the training data dataflow arguments as well. Otherwise, great updates and everything LGTM! 🏄‍♂️

CREATE_TRAINING_DATAFLOW_ARGS = COARSEN_RESTARTS_DATAFLOW_ARGS.copy()
CREATE_TRAINING_DATAFLOW_ARGS.update(
{
"--job_name": f"create-training-data-{getuser().lower()}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a unique tag here too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done thanks for catching

@brianhenn brianhenn merged commit 48d14e2 into master Mar 19, 2020
@brianhenn brianhenn deleted the feature/all-physics-baseline branch March 19, 2020 22:32
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

Successfully merging this pull request may close these issues.

Making the orchestrator work with arbitrary CLIs
3 participants