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
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions hamilton/graph.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@

Note: one should largely consider the code in this module to be "private".
"""
import collections
import inspect
import logging
import typing
from types import ModuleType
from typing import Type, Dict, Any, Callable, Tuple, Set, Collection, List
from typing import Type, Dict, Any, Callable, Tuple, Set, Collection, List, Mapping

import hamilton.function_modifiers_base
from hamilton import node
Expand Down Expand Up @@ -310,10 +311,10 @@ def dfs_traverse(node: node.Node):

@staticmethod
def execute_static(nodes: Collection[node.Node],
inputs: Dict[str, Any],
inputs: Mapping[str, Any],
adapter: base.HamiltonGraphAdapter,
computed: Dict[str, Any] = None,
overrides: Dict[str, Any] = None):
computed: Mapping[str, Any] = None,
overrides: Mapping[str, Any] = None):
Comment on lines +316 to +317
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.

"""Executes computation on the given graph, inputs, and memoized computation.

Effectively this is a "private" function and should be viewed as such.
Expand Down Expand Up @@ -369,7 +370,7 @@ def dfs_traverse(node: node.Node, dependency_type: DependencyType = DependencyTy
return computed

@staticmethod
def combine_config_and_inputs(config: Dict[str, Any], inputs: Dict[str, Any]) -> Dict[str, Any]:
def combine_config_and_inputs(config: Mapping[str, Any], inputs: Mapping[str, Any]) -> typing.Mapping[str, Any]:
"""Validates and combines config and inputs, ensuring that they're mutually disjoint.
:param config: Config to construct, run the DAG with.
:param inputs: Inputs to run the DAG on at runtime
Expand All @@ -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.


def execute(self,
nodes: Collection[node.Node] = None,
Expand Down