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

Add allow_module_overrides to AsyncDriver #1217

Conversation

rwhitten577
Copy link
Contributor

AsyncDriver got left out of the .allow_module_overrides feature. Adding support for it .
Fixes #1216

Changes

  • Pass allow_module_overrides from AsyncBuilder -> AsyncDriver -> Driver

How I tested this

  • Added new unit test to cover functionality
  • Verified in my own project & DAG that it works

Notes

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.

Copy link
Contributor

sweep-ai bot commented Nov 5, 2024

Hey @rwhitten577, here is an example of how you can ask me to improve this pull request:

@Sweep Add a unit test for AsyncDriver that verifies the behavior when multiple modules with the same function name are provided directly to the constructor (rather than through the Builder), testing both with and without allow_module_overrides=True.

📖 For more information on how to use Sweep, please read our documentation.

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! Reviewed everything up to 0d3db5d in 6 seconds

More details
  • Looked at 74 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. tests/test_async_driver.py:330
  • Draft comment:
    Consider adding a comment to explain the purpose of this test and the expected behavior when allow_module_overrides is used. This will improve the readability and maintainability of the test.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    The PR adds a new parameter allow_module_overrides to the AsyncDriver class and ensures it is passed correctly from AsyncBuilder. The test test_async_builder_allow_module_overrides verifies this functionality. However, the test could be improved by adding a comment to explain the purpose of the test and the expected behavior when allow_module_overrides is used.
2. hamilton/async_driver.py:201
  • Draft comment:
    The parameter allow_module_overrides is accessed as _allow_module_overrides in the Builder class. Consider using consistent naming across classes to avoid confusion.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code in async_driver.py and test_async_driver.py is generally well-structured and follows the given principles. However, there is a minor issue with the naming consistency of the allow_module_overrides parameter in the AsyncDriver class. The parameter is named allow_module_overrides in the AsyncDriver class, but it is accessed as _allow_module_overrides in the Builder class. This inconsistency could lead to confusion. It would be better to maintain consistent naming across the classes.

Workflow ID: wflow_h1iyTFyWEsERaH3Q


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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.

Looks good! Would be nice to have a test for build_without_init (could just add this to the one you have, or create another), but that's not necessary

@rwhitten577
Copy link
Contributor Author

Looks good! Would be nice to have a test for build_without_init (could just add this to the one you have, or create another), but that's not necessary

Thanks @elijahbenizzy , just added!

@elijahbenizzy elijahbenizzy merged commit a617bbd into DAGWorks-Inc:main Nov 6, 2024
24 checks passed
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.

allow_module_overrides doesn't work with AsyncBuilder
2 participants