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

Fix validation of campaign configs #143

Merged
merged 2 commits into from
Oct 7, 2021

Conversation

xylar
Copy link
Contributor

@xylar xylar commented Oct 6, 2021

This merge switches to first merging user and campaign config files and only validating after the merge. This slightly complicates the logic for getting the campaign (if any) because defaults have not yet been parsed at that point. But it does mean we only validate once (so only one validation success is printed) and it means that defaults will not override the campaign config options, as was happening before this merge.

@xylar xylar added semver: bug Bug fix (will increment patch version) for next version labels Oct 6, 2021
@xylar xylar requested a review from forsyth2 October 6, 2021 19:55
@xylar
Copy link
Contributor Author

xylar commented Oct 6, 2021

Without this fix, I'm seeing an error when I use the campaign = "water_cycle" config option:

$ zppy -c test.cfg 
Configuration file validation passed
Validation results={'e3sm_diags': True, 'e3sm_diags_vs_model': True, 'mpas_analysis': True, 'default': {'input': False, 'input_subdir': True, 'output': False, 'case': False, 'www': False, 'debug': True, 'partition': True, 'e3sm_unified': True, 'dry_run': True, 'campaign': True}, 'climo': True, 'ts': True, 'amwg': True, 'global_time_series': True}
Traceback (most recent call last):
  File "/home/ac.xylar/chrysalis/miniconda3/envs/zppy_dev/bin/zppy", line 33, in <module>
    sys.exit(load_entry_point('zppy', 'console_scripts', 'zppy')())
  File "/gpfs/fs1/home/ac.xylar/e3sm_work/zppy/add_cryo_config/zppy/__main__.py", line 43, in main
    _validate_config(config)
  File "/gpfs/fs1/home/ac.xylar/e3sm_work/zppy/add_cryo_config/zppy/__main__.py", line 119, in _validate_config
    raise Exception("Configuration file validation failed")
Exception: Configuration file validation failed

@xylar
Copy link
Contributor Author

xylar commented Oct 6, 2021

I'm not sure how this was missed in my testing of #138.

@xylar
Copy link
Contributor Author

xylar commented Oct 6, 2021

I'm testing this as part of my work on adding the cryosphere campaign. @forsyth2, it would be great if you could test as well.

@forsyth2
Copy link
Collaborator

forsyth2 commented Oct 6, 2021

I'm not sure how this was missed in my testing of #138.

That is strange. I wonder how we both missed that. I'm able to reproduce the error now.

@forsyth2, it would be great if you could test as well.

Using this change, configuration validation passes (although this statement prints twice, as I suppose would be expected) and the jobs are launched. Thanks for making this change!

@xylar
Copy link
Contributor Author

xylar commented Oct 6, 2021

(although this statement prints twice, as I suppose would be expected)

Yes. If that bothers you, we probably don't need to print out anything if things work as expected. Most users won't care about the details if there isn't a problem.

@xylar
Copy link
Contributor Author

xylar commented Oct 6, 2021

@forsyth2, let me know if you want me to include that change.

@forsyth2
Copy link
Collaborator

forsyth2 commented Oct 6, 2021

@forsyth2, let me know if you want me to include that change.

@xylar Yeah, if you don't mind, that would be great. Thanks! I personally like seeing the "validation passed" statement, but yes, probably most users won't care as long as it works.

@xylar xylar marked this pull request as draft October 6, 2021 21:49
@xylar
Copy link
Contributor Author

xylar commented Oct 6, 2021

@forsyth2, I'm running into more issues as I test the cryosphere configuration. The testing with the water cycle configuration was a little too simple because all the options in water_cycle.cfg are the same as in default.ini. As soon as that's not the case, I'm discovering conflicts where defaults are overriding campaign options rather than the other way around.

So I'm putting this PR in draft mode and will push some other fixes, hopefully soon.

@xylar xylar force-pushed the fix_campaign_config_validation branch 2 times, most recently from c9abfc6 to 42434c4 Compare October 6, 2021 22:11
This merge switches to first merging user and campaign config
files and only validating after the merge.  This slightly
complicates the logic for getting the campaign (if any) because
defaults have not yet been parsed at that point.  But it does
mean we only validate once (so only one validation success is
printed) and it means that defaults will not override the
campaign config options, as was happening before this merge.
@xylar xylar force-pushed the fix_campaign_config_validation branch from 42434c4 to 5bc7af5 Compare October 6, 2021 22:12
@xylar xylar marked this pull request as ready for review October 6, 2021 22:27
@xylar
Copy link
Contributor Author

xylar commented Oct 6, 2021

@forsyth2, I think this working correctly now and ready to review, based on my testing as part of #144

Copy link
Collaborator

@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.

Thanks @xylar. I tested on the following four cases and everything seems to work.

  • campaign = "water_cycle"
  • campaign = "none"
  • Campaign unspecified
  • campaign = "water_cycle" overriding sets as sets = 'qbo',

@xylar
Copy link
Contributor Author

xylar commented Oct 7, 2021

@forsyth2, thanks very much for testing. I think you can merge this whenever you're ready.

@forsyth2
Copy link
Collaborator

forsyth2 commented Oct 7, 2021

@xylar I'm working on some automated tests for this. Is it ok for me to push them to your branch before merging?

@forsyth2
Copy link
Collaborator

forsyth2 commented Oct 7, 2021

The commit I would add to this branch is here: forsyth2@245b735

@xylar
Copy link
Contributor Author

xylar commented Oct 7, 2021

@forsyth2, that looks good to me.

@forsyth2 forsyth2 merged commit 15fce90 into E3SM-Project:main Oct 7, 2021
@xylar xylar deleted the fix_campaign_config_validation branch October 7, 2021 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: bug Bug fix (will increment patch version)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants