Skip to content
This repository has been archived by the owner on Jul 3, 2023. It is now read-only.

Lazy config evaluation #64

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

elijahbenizzy
Copy link
Collaborator

[Short description explaining the high-level reason for the pull request]

Additions

Removals

Changes

Testing

Screenshots

If applicable

Notes

Todos

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows the standards laid out in the TODO link to standards
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Testing checklist

Python

  • python 3.6
  • python 3.7

@elijahbenizzy elijahbenizzy marked this pull request as draft February 7, 2022 03:51
This is more generic, and allows us to support lazily evaluated
configurations if needed.
@elijahbenizzy
Copy link
Collaborator Author

Need to add tests/figure out what to test. Hopefully this demonstrates the issue. IMO chainmap is actually better here -- supporting a lazyconfig is reasonable. That said, I'd prefer the simplicity with a dictionary.

@elijahbenizzy
Copy link
Collaborator Author

Pros of this:

  1. It allows lazily evaluated configs
  2. Its idiomatic -- types are only as constricted as they need to be (E.G. why does config have to be a dict?)
  3. It solves the problem a customer has

Cons of this:

  1. We can do less with the config/inputs - E.G. if we want to store them somewhere or print them out we'll be running up against the same problem.
  2. Added complexity for a specific use-case

Comment on lines +316 to +317
computed: Mapping[str, Any] = None,
overrides: Mapping[str, Any] = None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

do these need to change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, but I figured it would be nice to stay consistent.

@@ -379,7 +380,7 @@ def combine_config_and_inputs(config: Dict[str, Any], inputs: Dict[str, Any]) ->
duplicated_inputs = [key for key in inputs if key in config]
if len(duplicated_inputs) > 0:
raise ValueError(f'The following inputs are present in both config and inputs. They must be mutually disjoint. {duplicated_inputs}')
return {**config, **inputs}
return collections.ChainMap(config, inputs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

does the unit test need to change to enforce chain map's way of not evaluating things?

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, if the contract is that we don't evaluate the driver then we want to test that its not evaluated. That said, I don't really like that as a contract, and it feels limiting later on, so let's think through.

gitbook-com bot pushed a commit that referenced this pull request Feb 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants