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

Feature: Allow config params to be passable at the CLI and have a parameter_default #130

Merged
merged 5 commits into from
Nov 12, 2024

Conversation

johncmerfeld
Copy link
Contributor

From this Monday item:

Earthmover doesn't support the following construction:

config:
  output_dir: ${OUTPUT_DIR}
  parameter_defaults:
    OUTPUT_DIR: './output/'

(result is the creation of a directory named ${OUTPUT_DIR})

This PR adds support for this. The use case is for things we want to allow advanced users to configure but usually has a sensible default (e.g. OUTPUT_DIR, STATE_DIR)

Behavior notes:

  • Params passed at runtime still take precedence, and the default is only used if no param is passed. If the user provides an OUTPUT_DIR, the default is ignored.
  • Only "typical" environment variables are allowed, i.e. ${VALUE_X}. No spaces, dashes, etc. Just write it the way we normally do and you're fine.

You can test this easily with earthmover -t, which has been updated to utilize this construction. You can also modify the earthmover.yaml of an assessment bundle and run with its sample data.

@johncmerfeld johncmerfeld self-assigned this Oct 1, 2024
earthmover/earthmover.py Outdated Show resolved Hide resolved
# Combine `key: ${VAL}` with `VAL: default_val` to get `key: default_val`
val_as_env_var = val.replace("$", "").replace("{", "").replace("}", "")
if val_as_env_var in self.params:
configs[key] = self.params[val_as_env_var]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we use string templates here instead? something like

from string import template

for key, val in configs.items():
    if isinstance(val, str):
        template = Template(val)
        configs[key]= template.substitute(self.params)

This would eliminate the need for regex matching, and also support using an env var as part of the config value but not the whole value, like

config:
  output_dir: ${OUTPUT_BASE}transformed/
  parameter_defaults:
    OUTPUT_BASE: './output/'

Copy link
Contributor Author

@johncmerfeld johncmerfeld Oct 31, 2024

Choose a reason for hiding this comment

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

WOW, dramatically simpler, this is awesome, thanks! Tested that it works like a charm

earthmover/earthmover.py Show resolved Hide resolved
@johncmerfeld johncmerfeld requested a review from tomreitz October 31, 2024 17:19
Copy link
Collaborator

@tomreitz tomreitz left a comment

Choose a reason for hiding this comment

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

Looks great - thanks @johncmerfeld.

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