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

Parse config files with input envars #35

Merged
merged 5 commits into from
Jul 4, 2024

Conversation

DavidHuber-NOAA
Copy link
Collaborator

@DavidHuber-NOAA DavidHuber-NOAA commented Jun 27, 2024

Description

This allows the preconditioned sourcing of scripts based on optional environmental variables passed to the parse_config method within the Configuration class. A new test is added for the feature.

This is a required feature for NOAA-EMC/global-workflow#2693 to be able to source configs with RUN specified in order to get RUN-specific resources.

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

Created an xml with global-workflow branch https://github.com/DavidHuber-NOAA/global-workflow/tree/feature/simplify_res

  • pynorms
  • pytests

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes need updates to the documentation. I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • New and existing tests pass with my changes
  • Any dependent changes have been merged and published

Copy link

codecov bot commented Jun 27, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 3 lines in your changes missing coverage. Please review.

Project coverage is 49.93%. Comparing base (5dad7dd) to head (29c1a3c).
Report is 1 commits behind head on develop.

Files Patch % Lines
src/wxflow/configuration.py 66.66% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop      #35      +/-   ##
===========================================
+ Coverage    48.23%   49.93%   +1.70%     
===========================================
  Files           18       18              
  Lines         1644     1650       +6     
  Branches       335      337       +2     
===========================================
+ Hits           793      824      +31     
+ Misses         791      765      -26     
- Partials        60       61       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aerorahul
Copy link
Contributor

Is that really appropriate for a method that is supposed to just source a shell script and return the set variables as a dictionary.

This PR extends that from parsing/sourcing shell scripts and adding additional variables to that dictionary.

IMO that should be done after the sourcing and adding/updating the envvars.

@DavidHuber-NOAA
Copy link
Collaborator Author

@aerorahul I agree with your assessment on the capability. However, modifying the variables after sourcing is not enough for the global-workflow resources. They depend on knowing the RUN at the time of sourcing, otherwise those variables will be set based on the default RUN (gfs).

I see one possible solution:
Read the variables in the input config(s) and check for the existence of the input envvars. If they do not exist, raise an error. If they do exist, export the input envvars and source again.

An option that seems viable but is not:
In the global workflow at setup time, source config.base once, then modify the RUN in memory before sourcing task configs. Unfortunately, this will not work unless the contents of config.base are exported to bash as sourcing the task configs requires knowing the contents of config.base. This leads us back to a wxflow modification.

@DavidHuber-NOAA
Copy link
Collaborator Author

After offline discussion with @aerorahul, found a solution in the global-workflow to handle this case. Closing.

@DavidHuber-NOAA
Copy link
Collaborator Author

After testing of another option, this was found to be the favorable one. Reopening.

@DavidHuber-NOAA
Copy link
Collaborator Author

Pytests were run on WCOSS2 successfully.

Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

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

Looks good.

@aerorahul aerorahul merged commit d314e06 into NOAA-EMC:develop Jul 4, 2024
15 checks passed
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.

None yet

2 participants