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

Use click to allow command line arguments #79

Merged
merged 6 commits into from
Mar 8, 2024

Conversation

mnlevy1981
Copy link
Collaborator

Sorry about the misleading branch name, but click seems better suited to this task than argparse since pyproject.toml wants to call run() from cupid/run.py rather than running run.py as a command line script.

Fixes #72

Click seems better suited to this task than argparse since pyproject.toml wants
to call run() from cupid/run.py rather than running run.py as a command line
script.

First pass added --serial and --time-series flags; the former can be updated to
skip creating the LocalCluster object in notebooks, the latter can be updated
to run time series generation directly.
Also make use of serial in ocean and ice notebooks
click is already in the cupid-dev environment, but it was not listed as a
dependency in the pyproject.toml file
@mnlevy1981 mnlevy1981 marked this pull request as ready for review March 8, 2024 17:11
For now, this option raises a NotImplementedError exception. The branch that
incorporates time series should remove that block (as well as the "import sys"
line)
@mnlevy1981 mnlevy1981 requested a review from rmshkv March 8, 2024 21:18
@mnlevy1981
Copy link
Collaborator Author

mnlevy1981 commented Mar 8, 2024

(cupid-dev) $ cupid-run -h
Usage: cupid-run [OPTIONS] CONFIG_PATH

  Main engine to set up running all the notebooks.

Options:
  -s, --serial        Do not use LocalCluster objects
  -ts, --time-series  Run time series generation scripts prior to diagnostics
  -h, --help          Show this message and exit.

A few notes:

  1. cupid-run config.yml will create a LocalCluster in the ocean and sea ice notebooks
  2. cupid-run --serial config.yml will not create the LocalCluster
  3. cupid-run --time-series config.yml is not supported:
   (cupid-dev) $ cupid-run --time-series config.yml
   NotImplementedError: --time-series option not implemented yet
   (cupid-dev) $

@mnlevy1981 mnlevy1981 requested a review from TeaganKing March 8, 2024 21:21
@TeaganKing
Copy link
Collaborator

Should we also include the flag options in the README, or at least mention that descriptions are available with -h?

@mnlevy1981
Copy link
Collaborator Author

Should we also include the flag options in the README, or at least mention that descriptions are available with -h?

Yes! I'll update the README now.

Copy link
Contributor

@rmshkv rmshkv left a comment

Choose a reason for hiding this comment

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

This is a good first pass at these features, thanks for diving into this Mike!

Another thought is we probably want to include some more extensive documentation for people developing notebooks about including the serial parameter and logic with the local cluster, maybe in a separate page about parallelization, but I don't think that has to happen with this PR.

Copy link
Collaborator

@TeaganKing TeaganKing left a comment

Choose a reason for hiding this comment

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

Great! All these options are working for me. Thanks for adding a bit of info on this to the README. I agree with Lev's comment above, too.

@TeaganKing
Copy link
Collaborator

I made a note in #73 about documentation, so will merge and close this.

@TeaganKing TeaganKing merged commit 833eb95 into NCAR:main Mar 8, 2024
@mnlevy1981 mnlevy1981 deleted the add_argparse branch June 5, 2024 15:48
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.

Add flags to cupid-run
3 participants