-
Notifications
You must be signed in to change notification settings - Fork 21
Platform yaml #682
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
Open
uramirez8707
wants to merge
5
commits into
NOAA-GFDL:main
Choose a base branch
from
uramirez8707:platform_yaml
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Platform yaml #682
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
dfd2d1b
Fix to allow for platforms to contain fre_properties that can be used…
uramirez8707 8b81722
clean up any fre_properties from the platform yaml
uramirez8707 29c1812
Fix error messages
uramirez8707 efdbec0
Safely handle case when there are no platforms (i.e pp yamls)
uramirez8707 2b6a84b
#everythingworks
uramirez8707 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -121,9 +121,9 @@ def combine_platforms(self, yaml_content): | |
| :param yaml_content: string of yaml information including name, platform, | ||
| target, model, and compile yaml content | ||
| :type yaml_content: str | ||
| :return: dictionary of yaml information including name, platform, | ||
| target, model, compile, and platform yaml content | ||
| :rtype: dict | ||
| :return: string of yaml information including name, platform, | ||
| target, model, and compile yaml content | ||
| :rtype: str | ||
| """ | ||
| self.mainyaml_dir = os.path.dirname(self.yml) | ||
|
|
||
|
|
@@ -139,12 +139,9 @@ def combine_platforms(self, yaml_content): | |
| # Combine information as strings | ||
| yaml_content += platform_content | ||
|
|
||
| # Load string as yaml | ||
| yml = yaml.load(yaml_content, Loader = yaml.Loader) | ||
|
|
||
| # Return the combined string and loaded yaml | ||
| fre_logger.info(f" platforms yaml: {py_path}") | ||
| return yml | ||
| return yaml_content | ||
|
|
||
| def combine(self): | ||
| """ | ||
|
|
@@ -168,19 +165,22 @@ def combine(self): | |
|
|
||
| # Merge compile into combined file to create updated yaml_content/yaml | ||
| try: | ||
| yaml_content = self.combine_compile(yaml_content) | ||
| yaml_content = self.combine_platforms(yaml_content) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though not necessary, it might be nice to have the functions in the class above follow how the yamls are combined here, which means 2 changes:
|
||
| except Exception as exc: | ||
| raise ValueError("ERR: Could not merge compile yaml config with model config.") from exc | ||
| raise ValueError("ERR: Could not merge platform yaml config with model config.") from exc | ||
|
|
||
| # Merge platforms.yaml into combined file | ||
| try: | ||
| full_combined = self.combine_platforms(yaml_content) | ||
| full_combined = self.combine_compile(yaml_content) | ||
| except Exception as exc: | ||
| raise ValueError("ERR: Could not merge platform yaml config with model and compile configs.") from exc | ||
| raise ValueError("ERR: Could not merge compile yaml config with model and platform configs.") from exc | ||
|
|
||
| # Load string as yaml | ||
| yml = yaml.load(full_combined, Loader = yaml.Loader) | ||
|
|
||
| # Clean the yaml | ||
| try: | ||
| cleaned_yaml = clean_yaml(full_combined) | ||
| cleaned_yaml = clean_yaml(yml) | ||
| except Exception as exc: | ||
| raise ValueError("The final YAML could not cleaned.") from exc | ||
|
|
||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropping a test like this into test_helpers.py might help out the code coverage report: