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

Add config validation CLI #191

Merged
merged 1 commit into from
Nov 16, 2023
Merged

Conversation

SeanBryan51
Copy link
Collaborator

Fixes #186

@SeanBryan51 SeanBryan51 linked an issue Oct 26, 2023 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Merging #191 (505bc50) into main (474d183) will decrease coverage by 0.01%.
Report is 1 commits behind head on main.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##             main     #191      +/-   ##
==========================================
- Coverage   85.19%   85.19%   -0.01%     
==========================================
  Files          27       27              
  Lines        1385     1391       +6     
==========================================
+ Hits         1180     1185       +5     
- Misses        205      206       +1     
Files Coverage Δ
benchcab/cli.py 100.00% <100.00%> (ø)
tests/test_cli.py 100.00% <100.00%> (ø)
benchcab/benchcab.py 23.42% <50.00%> (+0.30%) ⬆️

@SeanBryan51
Copy link
Collaborator Author

SeanBryan51 commented Oct 26, 2023

This is a bit of a hack since the validation occurs in Benchcab.__init__() which will consequently run anything else that is inside the __init__ method. E.g. benchcab will yell at you if we are not running on Gadi due to the _validate_environment method being called.

Perhaps this issue will go away once we are able to port benchcab to other machines?

Copy link
Collaborator

@bschroeter bschroeter left a comment

Choose a reason for hiding this comment

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

So there is a way that this can be improved using a "parse and dispatch" strategy. Whereby you don't need to run a conditional to work out which subcommand to use.

See: https://github.com/AusClimateService/axiom/blob/54de78d3c490800bdf6806e15a1e8d23f2e82430/bin/axiom#L207

I basically set the desired command as a default on the sub parser and then pop it to call when dispatching.

https://github.com/AusClimateService/axiom/blob/54de78d3c490800bdf6806e15a1e8d23f2e82430/bin/axiom#L15

@@ -290,6 +290,9 @@ def main(self):
if self.args.subcommand == "run":
self.run()

if self.args.subcommand == "validate_config":
pass # handle validation in __init__ method
Copy link
Collaborator

Choose a reason for hiding this comment

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

As per yesterdays meeting, there is a validate_config method now present in config.py.

def validate_config(config: dict) -> bool:

Returns True if valid and raises an exception if not.

It would be worthwhile wrapping this in a local method to return something useful for the user in either case.

@SeanBryan51
Copy link
Collaborator Author

The parse and dispatch strategy looks like a good option. I've made a new issue for this (see #196) as it is a bit out of the scope of this PR and a bit more involved.

@ccarouge
Copy link
Collaborator

ccarouge commented Nov 9, 2023

I am wondering if it is time to introduce click. It's quite a bit of work so probably not necessary right away but it should remove the needs for either conditionals or a parse and dispatch strategy.

@SeanBryan51 SeanBryan51 merged commit adea352 into main Nov 16, 2023
2 of 4 checks passed
@SeanBryan51 SeanBryan51 deleted the 186-add-config-validation-cli branch November 16, 2023 23:51
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 config validation CLI.
3 participants