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

Improve validation error #320

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Improve validation error #320

wants to merge 1 commit into from

Conversation

forsyth2
Copy link
Collaborator

@forsyth2 forsyth2 commented Oct 5, 2022

Improve validation error. Resolves #281. Inadvertently discovered #319 in the process.

@forsyth2 forsyth2 added semver: bug Bug fix (will increment patch version) semver: small improvement Small improvement (will increment patch version) priority: high High priority task labels Oct 5, 2022
@forsyth2 forsyth2 added this to the December 2022 Requirements milestone Oct 5, 2022
@forsyth2 forsyth2 self-assigned this Oct 5, 2022
Copy link
Collaborator Author

@forsyth2 forsyth2 left a comment

Choose a reason for hiding this comment

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

It appears the behavior of config.validate(validator) has changed significantly.

@@ -13,7 +13,7 @@ www = "#expand user_www#zppy_test_complete_run_www"
[climo]
active = True
walltime = "00:30:00"
years = "1850:1854:2", "1850:1854:4",
years = "1850:1854:2", "1850:1854:4"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Launches the following:

climo_atm_monthly_180x360_aave_1850-1851
climo_atm_monthly_180x360_aave_1852-1853
climo_atm_monthly_180x360_aave_1850-1853
climo_atm_monthly_diurnal_8xdaily_180x360_aave_1850-1851
climo_atm_monthly_diurnal_8xdaily_180x360_aave_1852-1853
climo_atm_monthly_diurnal_8xdaily_180x360_aave_1850-1853

Both the "1850:1854:2" (2-year increment) jobs and the "1850:1854:4" (4-year increment) jobs are launched successfully. Does this mean the trailing comma is no longer a requirement of string_list parameters?

@@ -25,9 +25,9 @@ years = "1850:1854:2", "1850:1854:4",
vars = "PRECT"

[ts]
active = True
active = 4
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Causes ValueError: Invalid value 4 for 'active' at runtime rather than a failed configuration validation.

walltime = "00:30:00"
years = "1850:1854:2",
years = "1850:1854:2"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If active = True above, then the error we get here is ValueError: Error interpreting years 1. climo's multiple year sets above no longer appear to need a trailing comma. However, this error seems to indicate at least one comma must be present in a string_list parameter.

@@ -167,8 +168,12 @@ def _validate_config(config):
validator = Validator()

result = config.validate(validator)
print(type(result))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

result is a bool now. In the past, it has historically been some sort of dictionary or dictionary-like object. For example, from a validation failure discussed in a 3/22 Slack message, we had:

Validation results={'e3sm_diags': True, 'e3sm_diags_vs_model': True, 'mpas_analysis': True, 'default': True, 'climo': {'qos': True, 'nodes': True, 'walltime': True, 'input_files': True, 'fre
quency': True, 'mapping_file': True, 'grid': True, 'years': False, 'exclude': True, 'vars': True, 'atm_monthly_180x360_aave': True}, 'ts': True, 'tc_analysis': True, 'amwg': True, 'global_ti
me_series': {'input_subdir': True, 'qos': True, 'nodes': True, 'walltime': True, 'color': True, 'years': True, 'ts_num_years': True, 'figstr': True, 'moc_file': True, 'experiment_name': True
, 'ts_years': False, 'climo_years': False, 'atmosphere_only': True}}

@forsyth2
Copy link
Collaborator Author

forsyth2 commented Oct 5, 2022

It appears the behavior of config.validate(validator) has changed significantly.

@golaz See above comments regarding new issues with the validator. I can't find too much about the validate function. https://configobj.readthedocs.io/en/latest/validate.html# was last updated in 2014. https://github.com/DiffSK/configobj was last updated in 2020.

@forsyth2 forsyth2 removed this from the December 2022 Requirements milestone Oct 6, 2022
@forsyth2 forsyth2 mentioned this pull request Nov 1, 2022
@forsyth2
Copy link
Collaborator Author

forsyth2 commented Nov 1, 2022

I turned my branch of this into years-fix for #338. #281/#320 were intended to improve the error description from the validator. However, as reported in #319, the validator no longer warns of a missing comma. Therefore, it is simpler to just add the workaround in #338.

Ideally, we would get the validator working again (#319), at which point we can improve its error message (#281/#320), so I'm leaving this pull request open, but removing it from the December 2022 project.

@forsyth2 forsyth2 removed semver: bug Bug fix (will increment patch version) priority: high High priority task labels Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: small improvement Small improvement (will increment patch version)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve validation results error description
1 participant