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

CLI scaffolding #4

Merged
merged 1 commit into from
Aug 8, 2024
Merged

CLI scaffolding #4

merged 1 commit into from
Aug 8, 2024

Conversation

drewoldag
Copy link
Collaborator

This is meant to be a starting point for the CLI work, and is meant to give us a place to start hanging more code. The next step after this is integrated in my mind is to implement the user configuration file support.

The current CLI will look something like this:

>>> fibad
usage: fibad [-h] [--version] [{train,predict}] ...

Fibad CLI

positional arguments:
  {train,predict}  Verb to execute
  args             Arguments for the verb

options:
  -h, --help       show this help message and exit
  --version        Show version

FIBAD is the Framework for Image-Based Anomaly Detection

Getting top level help

>>> fibad --help
fibad --help
usage: fibad [-h] [--version] [{train,predict}] ...

Fibad CLI

positional arguments:
  {train,predict}  Verb to execute
  args             Arguments for the verb

options:
  -h, --help       show this help message and exit
  --version        Show version

FIBAD is the Framework for Image-Based Anomaly Detection

Running a fibad action:

>>> fibad train
Prending to run training...
Runtime config: None

Action-specific help

>>> fibad train --help
usage: fibad-train [-h] [-c RUNTIME_CONFIG]

Training with Fibad.

options:
  -h, --help            show this help message and exit
  -c RUNTIME_CONFIG, --runtime-config RUNTIME_CONFIG
                        Full path to runtime config file

Passing in a config argument

>>> fibad predict --runtime-config='/some/dir/over/here.toml'
Prending to run prediction...
Runtime config: /some/dir/over/here.toml

This work is heavily inspired by the CLI work done for Sorcha here: https://github.com/dirac-institute/sorcha/blob/main/src/sorcha_cmdline/main.py

@drewoldag drewoldag self-assigned this Aug 7, 2024
@drewoldag drewoldag linked an issue Aug 7, 2024 that may be closed by this pull request
Copy link

github-actions bot commented Aug 7, 2024

Before [cf6aade] After [e0ac614] Ratio Benchmark (Parameter)
3.15±0.9s 1.59±0.9s ~0.50 benchmarks.time_computation
240 3.7k 15.40 benchmarks.mem_list

Click here to view all benchmarks.


parser = argparse.ArgumentParser(description=description, epilog=epilog)
parser.add_argument("--version", dest="version", action="store_true", help="Show version")
parser.add_argument("verb", nargs="?", choices=fibad_verbs, help="Verb to execute")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to use subparsers here and let the individual verbs define their own flags?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was one option, but I was leaning toward a simpler approach for now. It may make sense to go down that route. I still want to think a little more about what we were talking about on Tuesday - namely unlocking many or all of the keys defined in the config file as arguments.

If we go down that route, subparsers may be the right approach. But I'm a little unsure/apprehensive about the order of events needed to load the config file, then use the contents to dynamically generate the parser arguments. In the base case of just looking at fibad's default config file, this isn't too challenging - but as we open this up to plug-in style DataLoaders and Models, each with their own bespoke config parameters - then we have to do some code introspection to identify available classes, grab their default configs, merge that with the base default configs...

Perhaps I'm overcomplicating it 🤷 that's why it would be nice to chat it out 😄


parser = argparse.ArgumentParser(description="Training with Fibad.")

parser.add_argument("-c", "--runtime-config", type=str, help="Full path to runtime config file")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we intend for these config files to be substantially different from one another?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The major difference that comes to mind would probably be sub-options for the different kinds of models that are implemented within fibad. But those can still probably be handled from within one "generic type" of config file.

Copy link
Collaborator Author

@drewoldag drewoldag Aug 8, 2024

Choose a reason for hiding this comment

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

My initial thought here is that the configs should not be substantially different. At this point I would advocate for all the user defined config values to be in the same file, even if they aren't used for a particular action. One obvious follow up would be, why not move the --runtime-config argument up to the main.py. Perhaps we could, but it's kind of nice to see fibad train --help produce output that shows that it expects a config file.

I'm happy to hear counter points to that though. My opinion here isn't a super strong one 😃

Very hand-wavy, a config file might look something like the following. Note that here it includes configs for everything, even if it was just to be used for training now, or just prediction later.

[fibad]  # general configs for running fibad???
max_gpus = 4  # just an example

[train]  # general configs for training
data_loader = base_loader
model_name = drewtonian  # this is type of model, right?
num_epochs = 100  # again, just an example

[train.base_loader]
data_directory = /foo/bar/data
...

[train.drewtonian]  # parameters for this model
num_layers = 10
...

[predict]  # general configs for prediction
data_loader = base_loader

[predict.base_loader]
data_directory = /foo/baz/new_data

I would love to talk to you both more about the structure of the config file - what is going to be most intuitive and easiest to use... etc.

Copy link
Collaborator

@mtauraso mtauraso left a comment

Choose a reason for hiding this comment

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

A few questions, but nothing blocking IMO. This should get merged.

Copy link
Collaborator

@aritraghsh09 aritraghsh09 left a comment

Choose a reason for hiding this comment

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

No major science-facing comments from my end. This looks good to be merged!

@drewoldag drewoldag merged commit 2d5c9a5 into main Aug 8, 2024
7 checks passed
@drewoldag drewoldag deleted the issue/3/cli-scaffolding branch August 8, 2024 04:18
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.

Implement scaffolding for CLI
3 participants