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

9.configure script update #37

Merged
merged 15 commits into from
Mar 5, 2024
Merged

9.configure script update #37

merged 15 commits into from
Mar 5, 2024

Conversation

singhd789
Copy link
Collaborator

No description provided.

Dana Singh and others added 12 commits January 3, 2024 09:36
- add yaml validation
- add click usage
- include all source information for components in regrid rose-app (until rose is retired)
- configure script validates yaml and writes more information in rose-apps
- edits yaml updated to feel more like previous experiment XMLS
- schema.json updated to validate new yaml format
- accidentally added these files from a merge conflict that was fixed previously on branch
@singhd789 singhd789 marked this pull request as ready for review February 23, 2024 21:17
@singhd789
Copy link
Collaborator Author

Note: the configUpdates folder is not needed and will be taken out. The reason it is still in there in because of something Ian and I were looking at in our refactoring task, but it is solely for my development purposes.

@singhd789 singhd789 marked this pull request as draft February 27, 2024 14:56
configuration_paths:
rose-suite: "/home/Dana.Singh/pp/ppyaml/fre-cli/fre/frepp/configUpdates/rose-suite-am5_c96L33_amip.conf"
rose-remap: "/home/Dana.Singh/pp/ppyaml/fre-cli/fre/frepp/configUpdates/remap-rose-app.conf"
rose-regrid: "/home/Dana.Singh/pp/ppyaml/fre-cli/fre/frepp/configUpdates/regrid-rose-app.conf"
Copy link
Collaborator

Choose a reason for hiding this comment

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

There 3 files are the rewritten configurations, yes?

I think it would be better to have these paths passed to the fre pp configure tool rather than in the pp.yaml proper.

Partly because these paths are not "first-class" pp configuration and more internal settings, and partly as someone might want to apply the same pp.yaml to multiple workflows using the same pp.yaml:

fre pp configure -y ~/my-stock-pp.yaml -e am5_r1i1p1 -p gfdl.ncrc5-intel -t prod-openmp

fre pp configure -y ~/my-stock-pp.yaml -e am5_test -p gfdl.ncrc5-intel -t prod-openmp

chunk96: &PP_AMIP_CHUNK96 "1yr"
analysisStart: &ANA_AMIP_START "1980"
analysisStop: &ANA_AMIP_STOP "1988"
defxyInterp: &defaultxyInterp "180,288"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Neat... these are like FRE properties, user variables that can be reused later?

I see the value set there, e.g. 10yr, but is the variable analysisLen or ANA_AMIP_LEN?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes! They're used later in the components section. It was slightly confusing, the variable is defined as analysisLen in the yaml, but the value actually reused later, that saves the value the user sets, is the one with the &

rose-suite:
refineDiag:
datastager_script: "$(includeDir)/refineDiag/refineDiag_data_stager_globalAve.csh"
atmoscmip6_script: "$(includeDir)/refineDiag/refineDiag_atmos_cmip6.csh"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is "atmoscmip6_script" part of the yaml schema or a user setting?

Copy link
Collaborator Author

@singhd789 singhd789 Feb 27, 2024

Choose a reason for hiding this comment

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

Oh! This was originally following the draft you had started in the issue (#9). There were 2 refineDiag scripts but both were called script so I found that the latter was the only one found. Changing the names from script just let's us have access to both scripts. atmoscmip6_script would be part of the schema here though. Good catch, I should change it to be more general, like taking away the 6.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Quick clarifying question here, in the rose-suite.conf, the variable REFINEDIAG_SCRIPTS is sometimes defined (if do_refineDiag is True). Would that variable need to point to both of these scripts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. REFINEDIAG_SCRIPTS can be space-separated, and each filepath should be an absolute path to the script.

target: "prod-openmp"

#regrid-xy and remap-pp-components information
regrid-remap-info:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes! The components part down is perfect as is, clearly analogous to a Bronx XML and I think users will be able to understand it.

Can the regrid-remap-info header be removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it can. I put that in there for more organization but I'm sure it would work with just components as the header here.

}
},
"analysisLen": {"type": "string"},
"chunk96": {"type": "string"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

The name chunk96 is part of the schema, here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, the chunk96 was part of those user variables that could be reused later. This name could change

Copy link
Collaborator

Choose a reason for hiding this comment

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

User variable names shouldn't be in the pp.yaml schema, should they? Can users name their variables whatever they like?

Copy link
Collaborator Author

@singhd789 singhd789 Feb 29, 2024

Choose a reason for hiding this comment

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

Well in the pp.yaml, in order for it to be validated correctly, I think it should be defined as something. I can change it to something like pp_amip_chunk or something instead of chunk96. The actual variable that can be reused throughout the pp.yaml, i.e. &PP_AMIP_CHUNK96, can be called anything the user wants though, as long as it's consistent through the yaml.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I'm still confused. is the variable called pp_amip_chunk or PP_AMIP_CHUNK96?

Copy link
Collaborator Author

@singhd789 singhd789 Feb 29, 2024

Choose a reason for hiding this comment

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

You're good! That wasn't a great explanation on my part at all. I was looking for this link to help.

https://docs.geoserver.org/2.21.x/en/user/styling/ysld/reference/variables.html

With reusable yaml variables, it has this structure:
define: &variable <value>
Where:
define = pp_amip_chunk
$variable = &PP_AMIP_CHUNK96
(PP_AMIP_CHUNK96was what the variable was called in the XML, and the variable to be reused in the yaml later on in the components section)
<value> = "1yr"

To my understanding, I see the define (whatever we call it) to mainly be used to validate with the schema

Copy link
Collaborator

Choose a reason for hiding this comment

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

aha, thanks for that. I see. I agree that's just what we want.

Can't we have just one variable name, PP_AMIP_CHUNK96 that is user set? What does the lower case pp_amip_chunk in?

This is the stock example:

define: &variable <value>

Here's our example:

define: %PP_AMIP_CHUNK96 '1yr'

And then PP_AMIP_CHUNK96 can be used in the yaml below as:

*PP_AMIP_CHUNK96

Copy link
Collaborator

@ceblanton ceblanton left a comment

Choose a reason for hiding this comment

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

Dana, thank you so much for this. I can't follow all of it, and I think some of the pp.yaml could be simplified, but if mostly works now, I'm inclined to think we should merge it and begin to use and test it more. We can always adjust as we go.

@singhd789
Copy link
Collaborator Author

Thanks for taking a look through this Chris!

I attempted to test this before, to see if the configurations were actually good to run a workflow, but there were some error messages in the rose macro --validate step. I know this has it's own issues, but it brought to light that I may have forgot some variables or potentially set some incorrectly, like outputGridType or inputRealm in rose-apps. I'm in the process of looking through these.

@singhd789 singhd789 marked this pull request as ready for review March 5, 2024 20:42
@singhd789 singhd789 merged commit 0cdd5f7 into main Mar 5, 2024
1 check passed
@ilaflott ilaflott deleted the 9.configure-script-update branch June 18, 2024 17:52
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.

2 participants