Skip to content

make workflow configs parseable outside R and resolve input paths from env - #4082

Open
divine7022 wants to merge 13 commits into
PecanProject:developfrom
divine7022:n_ncc_tweaks
Open

make workflow configs parseable outside R and resolve input paths from env#4082
divine7022 wants to merge 13 commits into
PecanProject:developfrom
divine7022:n_ncc_tweaks

Conversation

@divine7022

Copy link
Copy Markdown
Member

Description

Motivation and Context

Review Time Estimate

  • Immediately
  • Within one week
  • When possible

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • My name is in the list of CITATION.cff
  • I agree that PEcAn Project may distribute my contribution under any or all of
    • the same license as the existing code,
    • and/or the BSD 3-clause license.
  • I have updated the CHANGELOG.md.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

- Only crop codes present in the crosswalk resolve to an N rate envelope. Cycles whose
code does not resolve are dropped and reported at run time.

# Running outside the BU cluster

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Edit this header to something more generic like "Config file".


# paths in config.yml are relative to ccmmf_dir so the yaml stays plain data any
# parser can read. a value from the environment wins and is used as given, so a
# run can point anywhere without editing the file

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd like to check in on the second assumption above -- from a design perspective, why would we prioritize values set from the environment over values within the file. Also, why would doing so somehow make the code more portable, since you just need other code to set all those environment variables to local values, which means that you now have two different files trying to set the same configs (config.yml and the script setting the env variables). This seems redundant, confusing, and error prone. There's also no reason a config file can't have the user define a root directory as an absolute path and then define other paths as relative paths based on that root (or even to detect whether the other path is absolute or relative)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I had a related discussion with @hdpriest-ui yesterday - setting values in config is preferred over using environment variables for portability and reproducibility.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But the use of environmental variables is widespread in the code so it isn't necessary or prudent to force refactoring working code in the short term.

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.

3 participants