64 add default parameter sweep to run notebook command#65
64 add default parameter sweep to run notebook command#65capn-freako wants to merge 2 commits intomasterfrom
Conversation
jdpatt
left a comment
jdpatt
left a comment
There was a problem hiding this comment.
I don't make use of this feature but giving feedback where I can!
| "Run a *Jupyter* notebook on an IBIS-AMI model file." | ||
| arguments_list = sys.argv | ||
| full_command_line = "run-notebook " + " ".join(shlex.quote(arg) for arg in arguments_list[1:]) | ||
| if out_dir: |
There was a problem hiding this comment.
We should just set this once. I would add an else that sets it to the ibis file and remove the second update of it in run_notebook.
if out_dir:
out_dir=Path(out_dir).resolve()
else:
out_dir = Path(ibis_file).resolve().parent
out_dir.mkdir(exist_ok=True)
There was a problem hiding this comment.
Please, check my next push, to see if I've correctly interpreted your suggestion.
Fixed in next push (I think).
| # Write parameter sweep specification template file. | ||
| root_name = str(pcfg.input_ami_params[ParamName("root_name")]) | ||
| run_file_path = Path(root_name + ".run").resolve() | ||
| with open(run_file_path, mode="wt", encoding="utf-8") as run_file: |
There was a problem hiding this comment.
I believe we already generate *.run files in the mk_tests function; can we extract this to a write_run_file(filepath, dict) helper that does the same for both? Just have to give each the proper formatted dictonary so if we make edits later; both outputs are lock step and not segmented. This can be a future improvement if required.
There was a problem hiding this comment.
Nice catch!
Yes, I'll factor out the commonality into a helper function, as you suggest.
Now, reading the docstring for mk_tests():
test_dir: Directory in which to place the created test run definition files.
Default: "test_runs/"
I'm wondering, wrt/ your first comment above, in the case where no run file has been provided and we're making a default one, as a convenience to the user, should we place it not in the current working directory, but rather in a subdirectory named test_runs?
There was a problem hiding this comment.
I think if the results and everything else goes into --out_dir which can be defined by the user, the user can specify they want it in test_runs vs. us making that decision. The only oddity will be when we generate the spec file for them, say I run in my script in C: but my out_dir is C:\projects\cool_sim. I'm going to have to move that spec file to be in the out_dir?
Maybe in addition, the user should be able to spec the location of the sweep file and that gets passed along as an optional arg and fall back to assuming its in out_dir?
| # Define temp. (i.e. - parameterized) notebook and output file locations. | ||
| tmp_dir = ( | ||
| os.environ.get("TMP") or os.environ.get("TEMP") or # noqa: W504 | ||
| (Path(os.environ.get("HOME")).joinpath("tmp") # type: ignore | ||
| if os.environ.get("HOME") | ||
| else "/tmp") | ||
| ) | ||
| tmp_dir = Path(tmp_dir) | ||
| tmp_dir.mkdir(exist_ok=True) | ||
| tmp_notebook = tmp_dir.joinpath(notebook.stem + "_papermill").with_suffix(".ipynb") |
There was a problem hiding this comment.
Just saw this when looking at your comments, not a change you made in this commit but when you need to make a temp file; take a look at https://docs.python.org/3/library/tempfile.html which is builtin and no need to figure out where temp actually is.
2 participants
No description provided.