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

Unify the node and dataflow versioning API #734

Merged
merged 13 commits into from
Mar 14, 2024
Merged

Unify the node and dataflow versioning API #734

merged 13 commits into from
Mar 14, 2024

Conversation

zilto
Copy link
Collaborator

@zilto zilto commented Mar 4, 2024

Motivation described in #728 and relates to #175.

Changes

  • moved the hash_source_code() and the utility remove_docs_and_comments() from grap_utils to graph_types
  • added to graph_types.HamiltonNode and graph_types.HamiltonGraph a .version property
  • updated code that depended on these functions and the related tests
  • updated the hamilton.plugins.h_experiments.hook.ExperimentTracker to use the HamiltonGraph.version implementation to align with CLI and DiskCache

How I tested this

  • ran the full test suite successfully after edits

Notes

  • some code path use the hash_source_code() directly on functions/Callables or on hamilton.node.Node

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • 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 if adding/changing functionality.

@zilto zilto requested a review from elijahbenizzy March 4, 2024 15:02
ellipsis-dev[bot]

This comment was marked as spam.

ellipsis-dev[bot]

This comment was marked as spam.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me!

  • Performed an incremental review on 248f191
  • Looked at 93 lines of code in 1 files
  • Took 1 minute and 37 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 50%.
1. tests/test_driver_export.py:109:
  • Assessed confidence : 0%
  • Comment:
    Good job on updating the test to dynamically set the version attribute of each node in the EXPECTED_JSON. This ensures that the test will pass regardless of the Python version used.
  • Reasoning:
    The test test_export_execution has been updated to reflect the changes in the PR. The version attribute of each node in the EXPECTED_JSON is now dynamically set to the version of the corresponding node in the graph. This is a good change as it ensures that the test will pass regardless of the Python version used, as the version attribute is dependent on the Python version. I don't see any issues with this change.

Workflow ID: wflow_9buXnjTWmOPF5dWT


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me!

  • Performed an incremental review on a60db44
  • Looked at 21 lines of code in 1 files
  • Took 2 minutes and 23 seconds to review
More info
  • Skipped 0 files when reviewing.
  • Skipped posting 1 additional comments because they didn't meet confidence threshold of 50%.
1. tests/test_driver_export.py:107:
  • Assessed confidence : 100%
  • Grade: 40%
  • Comment:
    Consider creating a deep copy of EXPECTED_JSON within the test and modify that instead of the global variable to avoid side effects.
  • Reasoning:
    The test test_export_execution modifies the global EXPECTED_JSON variable in each run. This could lead to unexpected results if multiple tests use this variable or if the test is run multiple times in the same session. It would be better to create a deep copy of EXPECTED_JSON within the test and modify that instead.

Workflow ID: wflow_2Otbj2mKo61mztca


Not what you expected? You can customize the content of the reviews using rules. Learn more here.

Copy link
Collaborator

@elijahbenizzy elijahbenizzy left a comment

Choose a reason for hiding this comment

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

A few thoughts, but overall good

tests/test_graph_types.py Show resolved Hide resolved
hamilton/graph_types.py Show resolved Hide resolved
hamilton/graph_types.py Show resolved Hide resolved
tests/test_driver_export.py Show resolved Hide resolved
tests/test_graph_types.py Show resolved Hide resolved
tests/test_graph_types.py Outdated Show resolved Hide resolved
tests/test_graph_types.py Outdated Show resolved Hide resolved
hamilton/graph_types.py Show resolved Hide resolved
@zilto zilto merged commit 65be67c into main Mar 14, 2024
23 checks passed
@zilto zilto deleted the feat/versioning branch March 14, 2024 13:26
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