Redesign command line interface by SeanBryan51 · Pull Request #59 · CABLE-LSM/benchcab

SeanBryan51 · 2023-04-10T23:58:46Z

Currently, benchcab uses two main program entry points: benchcab and benchsiterun. Having benchsiterun accessible at the same level as benchcab allows the user to run benchsiterun incorrectly, for example if the user tries to run benchsiterun before the work directory is generated. Ideally benchsiterun is invoked only by the job script submitted by benchcab and should be "hidden" to the user.

Move the benchsiterun main program entry point into benchcab.

Add the ability to run specific steps in the workflow in isolation via subcommands.

These steps include checkout, build, setting up the fluxnet task work directory and executing the fluxnet tasks.

Add the --no-submit flag to disable the job script submission when executing fluxnet tasks.

This flag can also be specified in the main workflow if the user wishes to execute tasks on the login node (if the computation is inexpensive enough so that the job doesn't get killed).

Add unit tests to test the command line arguments are parsed correctly.

This should provide a command line interface that makes it difficult for the user to incorrectly run benchsiterun (now benchcab fluxnet-run-tasks --no-submit).

Fixes #21

Testing

pytest success
pip install --user . success
benchcab fluxnet success
benchcab fluxnet --no-submit success (ran with a single science configuration and AU-Tum experiment)

Currently, benchcab uses two main program entry points: benchcab and benchsiterun. Having benchsiterun accessible at the same level as benchcab allows the user to run benchsiterun incorrectly, for example if the user tries to run benchsiterun before the work directory is generated. Ideally benchsiterun is invoked only by the job script submitted by benchcab and should be "hidden" to the user. Move the benchsiterun main program entry point into benchcab. Add the ability to run specific steps in the workflow in isolation via [subcommands]. These steps include checkout, build, setting up the fluxnet task work directory and executing the fluxnet tasks. Add the --no-submit flag to disable the job script submission when executing fluxnet tasks. This flag can also be specified in the main workflow if the user wishes to execute tasks on the login node (if the computation is inexpensive enough so that the job doesn't get killed). Add unit tests to test the command line arguments are parsed correctly. This should provide a command line interface that makes it difficult for the user to incorrectly run benchsiterun (now `benchcab fluxnet-run-tasks --no-submit`). Fixes #21 [subcommands]: https://docs.python.org/3/library/argparse.html#sub-commands

github-actions · 2023-04-10T23:59:13Z

PR Preview Action v1.2.0
Preview removed because the pull request was closed.
2023-04-27 01:38 UTC

ccarouge

I'm happy with sub-commands to do specific parts of the workflow. But I'd like to keep a default workflow that runs with a simple benchcab call. For me, the default would be to run the flux site case and the spatial case (once it exists). I.e. the default is to run a full workflow.

It would be great if we could get this behaviour back.

SeanBryan51 · 2023-04-14T03:51:26Z

@ccarouge I propose instead of having benchcab, we have benchcab run for executing the default workflow?
This way, subcommands such as checkout or build won't inherit optional arguments that are not needed from the default benchcab command. For example, passing the --no-submit flag to benchcab is valid but this design allows passing the same flag to benchcab checkout which doesn't really make sense.

I think sub commands is one of those things where if you start using them, everything needs to become a sub command 😅

ccarouge · 2023-04-14T04:31:48Z

Yes, benchcab run is ok. That's easy enough to remember.

@ccarouge

This keeps a default workflow that runs with a simple `benchcab run` call. The default workflow runs the full test suite (fluxnet, spatial, ...). Changes requested by @ccarouge: #59 (review)

ccarouge

Some minor modifications to fix but this can be merged after the fixes are applied to your liking.

ccarouge · 2023-04-17T05:43:59Z

+        benchcab_fluxnet(args, config, tasks=get_fluxnet_tasks(
+            realisations=config["realisations"],
+            science_config=config['science_configurations'],
+            met_sites=get_met_sites(config['experiment'])
+        ))
+        benchcab_spatial()


Suggested change

benchcab_fluxnet(args, config, tasks=get_fluxnet_tasks(

realisations=config["realisations"],

science_config=config['science_configurations'],

met_sites=get_met_sites(config['experiment'])

))

benchcab_spatial()

benchcab_run(args, config)

Let's use benchcab_run here instead

That's for the case if args.subcommand == 'run':

ccarouge · 2023-04-17T05:47:28Z

+    if args.subcommand == 'fluxnet-run-tasks':
+        benchcab_fluxnet_run_tasks(args, config, tasks=get_fluxnet_tasks(
+            realisations=config["realisations"],
+            science_config=config['science_configurations'],
+            met_sites=get_met_sites(config['experiment'])
+        ))
+


Would it be worth getting the list of tasks only once? I don't like having the call to get_fluxnet_tasks copied so many times. It's an easy line to mess up.

Maybe worth putting it after the call to validate_environment.

ccarouge · 2023-04-17T05:52:08Z

+        'run',
+        parents=[args_help, args_subcommand, args_run_subcommand],
+        help="Run all test suites for CABLE.",
+        description="""Runs all test suites for CABLE.""",


Suggested change

description="""Runs all test suites for CABLE.""",

description="""Runs all test suites for CABLE: fluxnet sites and spatial test suites. Use this command to run the full default set of tests for CABLE""",

I have tried to add something to say: "this is the main command to use." Feel free to find another way to do that.

ccarouge · 2023-04-17T05:54:20Z

+        description="""Runs the default fluxnet test suite for CABLE. To launch the test suite, run
+        'benchcab fluxnet'. This command is the equivalent of running 'benchcab checkout', 'benchcab


"""To launch the test suite, run 'benchcab fluxnet."""

This part of the description is only stated for this sub-command. Either remove or add the equivalent to all sub-commands.

ccarouge · 2023-04-17T06:00:09Z

+        help="Run the work directory setup step of the main fluxnet command.",
+        description="""Generates the benchcab work directory in the current working directory so
+        that tasks can be run.""",


Suggested change

help="Run the work directory setup step of the main fluxnet command.",

description="""Generates the benchcab work directory in the current working directory so

that tasks can be run.""",

help="Run the work directory setup step of the fluxnet command.",

description="""Generates the benchcab site/run directory tree in the current working directory so

that tasks can be run.""",

ccarouge · 2023-04-17T06:00:47Z

+        help="Run the fluxnet tasks of the main fluxnet command.",
+        description="""Runs the fluxnet tasks for the main fluxnet test suite. By default, this
+        command generates a PBS job script and submits it to the queue.""",
+        add_help=False


Suggested change

help="Run the fluxnet tasks of the main fluxnet command.",

description="""Runs the fluxnet tasks for the main fluxnet test suite. By default, this

command generates a PBS job script and submits it to the queue.""",

add_help=False

help="Run the fluxnet tasks of the fluxnet command.",

description="""Runs the fluxnet tasks for the fluxnet test suite. By default, this

command generates a PBS job script and submits it to the queue.""",

add_help=False

ccarouge · 2023-04-17T06:01:54Z

+        description="""Runs the default spatial test suite for CABLE. To launch the test suite,
+        run 'benchcab spatial'.""",


To launch the test suite, run 'benchcab spatial'.

Again, choose if this is stated for all commands or none.

@ccarouge

Changes requested by @ccarouge: #59 (review)

SeanBryan51 · 2023-04-18T00:46:08Z

@ccarouge I've made the changes you requested. Can you review the changes made in bb980bb? This change is addressing your comment: #59 (comment)

Would it be worth getting the list of tasks only once? I don't like having the call to get_fluxnet_tasks copied so many times. It's an easy line to mess up.

Maybe worth putting it after the call to validate_environment.

I thought putting get_fluxnet_tasks after the validate_environment line is not ideal as we would be calling it for every subcommand of benchcab. I've instead abstracted get_fluxnet_tasks into a single function _initialise_tasks which removes the repetition.

As for why I went with a class implementation: this was to reduce the repetition of common arguments between all the subcommands. This is all just for code aesthetic.

Edit: update commit hash

Although this change contains very minimal functional changes, it eliminates code repetition and is hopefully easier to maintain in the future. Lazily initialise fluxnet tasks only when needed for fluxnet specific commands.

ccarouge

I think we can leave it at this for now. I think the final design might take a bit of discussion and it's not a priority now.

ccarouge · 2023-04-26T06:51:24Z

+        if self.args.subcommand == 'run':
+            self.run()


As discussed before, I'm no big fan of this list of "if". I saw the use of set_default() which is probably the best solution but we would have to work around the circular dependency.

The other idea I have is this:

def main(self): subcommands={"run": self.run, "checkout": self.checkout} # Call subcommand requested subcommand[self.args.subcommand]()

The dictionary needs all the subcommands and it doesn't need to be defined in main. This is just to give the idea.

SeanBryan51 linked an issue Apr 10, 2023 that may be closed by this pull request

Should benchcab have only one main program entry point? #21

Closed

SeanBryan51 requested a review from ccarouge April 11, 2023 00:04

SeanBryan51 added 2 commits April 13, 2023 10:46

Merge branch 'master' into 21-replace-benchsiterun-entry-point

503cf8d

Capitalize help message for --version

29f6038

ccarouge requested changes Apr 13, 2023

View reviewed changes

Add benchcab run subcommand

d517b1d

This keeps a default workflow that runs with a simple `benchcab run` call. The default workflow runs the full test suite (fluxnet, spatial, ...). Changes requested by @ccarouge: #59 (review)

SeanBryan51 requested a review from ccarouge April 17, 2023 00:08

ccarouge approved these changes Apr 17, 2023

View reviewed changes

Update command line descriptions

8d86a85

Changes requested by @ccarouge: #59 (review)

SeanBryan51 requested a review from ccarouge April 18, 2023 00:34

Refactor main function into a class

bb980bb

Although this change contains very minimal functional changes, it eliminates code repetition and is hopefully easier to maintain in the future. Lazily initialise fluxnet tasks only when needed for fluxnet specific commands.

SeanBryan51 force-pushed the 21-replace-benchsiterun-entry-point branch from b8a9471 to bb980bb Compare April 18, 2023 03:16

ccarouge approved these changes Apr 26, 2023

View reviewed changes

SeanBryan51 merged commit 23bb243 into master Apr 27, 2023

SeanBryan51 deleted the 21-replace-benchsiterun-entry-point branch April 27, 2023 01:38

	description="""Runs all test suites for CABLE.""",
	description="""Runs all test suites for CABLE: fluxnet sites and spatial test suites. Use this command to run the full default set of tests for CABLE""",

		description="""Runs the default fluxnet test suite for CABLE. To launch the test suite, run
		'benchcab fluxnet'. This command is the equivalent of running 'benchcab checkout', 'benchcab

		description="""Runs the default spatial test suite for CABLE. To launch the test suite,
		run 'benchcab spatial'.""",

Conversation

SeanBryan51 commented Apr 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Uh oh!

github-actions Bot commented Apr 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ccarouge left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SeanBryan51 commented Apr 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ccarouge commented Apr 14, 2023

Uh oh!

ccarouge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SeanBryan51 commented Apr 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ccarouge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SeanBryan51 commented Apr 10, 2023 •

edited

Loading

github-actions Bot commented Apr 10, 2023 •

edited

Loading

ccarouge left a comment •

edited

Loading

SeanBryan51 commented Apr 14, 2023 •

edited

Loading

SeanBryan51 commented Apr 18, 2023 •

edited

Loading