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

Changes the way drivers handle parameters #56

Merged
merged 1 commit into from
Feb 6, 2022
Merged

Changes the way drivers handle parameters #56

merged 1 commit into from
Feb 6, 2022

Conversation

elijahbenizzy
Copy link
Collaborator

@elijahbenizzy elijahbenizzy commented Feb 5, 2022 •
edited
Loading

See #52

Note: this is backwards compatible. The changes are that
we've added the "inputs" parameter to the driver, as well as the
function_graph executor. This allows external inputs to the DAG
at runtime, not just construction time.

[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 force-pushed the fix-inputs branch 3 times, most recently from 69ab3f0 to 7eae784 Compare February 5, 2022 02:54
skrawcz
skrawcz reviewed Feb 6, 2022
View reviewed changes
hamilton/graph.py Outdated
return FunctionGraph.execute_static(
nodes=nodes,
inputs=self.config,
inputs={**inputs, **self.config},
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should make this a function, have it check for overlap and log/warn, and then unit test it.

Also this seems like it's the invariant approach?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep will do. And by that you mean backwards compatible?

Copy link
Collaborator

Choose a reason for hiding this comment

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

invariant -- that the config takes precedence over any inputs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh yeah. So conflicts are not allowed as it doesn't make sense (for now). So yes, invariant, but more just mutually disjoint.

skrawcz
skrawcz reviewed Feb 6, 2022
View reviewed changes
hamilton/driver.py Outdated Show resolved Hide resolved
@elijahbenizzy elijahbenizzy force-pushed the fix-inputs branch 2 times, most recently from 58760ca to 94bc686 Compare February 6, 2022 03:16
@elijahbenizzy elijahbenizzy mentioned this pull request Feb 6, 2022
Closed
11 tasks
Base automatically changed from decorator-refactor to main February 6, 2022 21:29
skrawcz
skrawcz reviewed Feb 6, 2022
View reviewed changes
hamilton/driver.py Outdated
Comment on lines 58 to 59
Don't rely on it too much. This might produce an extra error, but this is the first one that the user
should deal with.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Im confused, why would this be true? You just said that they're disjoint?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If no errors are returned from this function they're disjoint. If errors are returned, then you don't want to trust the result that's also returned. but as we want to get all errors at once its good not to short-circuit.

skrawcz
skrawcz reviewed Feb 6, 2022
View reviewed changes
tests/test_graph.py Outdated

def test_combine_inputs_collision_2():
"""Tests the combine_and_validate_inputs functionality
when there are collisions of keys but not values"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

and values?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

copy pasta :(

@elijahbenizzy
Changes the way drivers handle parameters
39dd55a 
See #52

Note: this is backwards compatible. The changes are that
we've added the "inputs" parameter to the driver, as well as the
function_graph executor. This allows external inputs to the DAG
at runtime, not just construction time.
@elijahbenizzy elijahbenizzy merged commit 9f6c793 into main Feb 6, 2022
@elijahbenizzy elijahbenizzy deleted the fix-inputs branch February 6, 2022 22:13
@skrawcz skrawcz linked an issue Feb 7, 2022 that may be closed by this pull request
Prototype Row Based Execution of a DAG #43
Closed
gitbook-com bot pushed a commit that referenced this pull request Feb 10, 2022
@elijahbenizzy @gitbook-bot
GitBook: [#56] No subject
4ddb761
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@skrawcz skrawcz skrawcz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Prototype Row Based Execution of a DAG
2 participants
@elijahbenizzy @skrawcz