Updates from_functions and from_modules to use the same base "compile" #1275
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 7d7fbfe in 1 minute and 27 seconds
More details
- Looked at
243lines of code in4files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. pyproject.toml:1
- Draft comment:
Overall, the pyproject.toml looks well organized. A minor note: the dependency for furo has been simplified from a Git URL to just 'furo'. Verify that this meets your version requirements; if a current release is critical, consider pinning a version or adding a note in the docs. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. pyproject.toml:10
- Draft comment:
Ensure that 'requires-python = ">=3.8, <4"' meets your expected Python support. This range may need updating if planning for future Python 4 releases. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. hamilton/graph.py:736
- Draft comment:
In FunctionGraph.compile() and related factory methods, the parameter 'allow_module_overrides' is passed into compile_to_nodes via the mismatched parameter 'allow_node_leveL_overrides'. It might be worth standardizing these names to reduce confusion. - Reason this comment was not posted:
Marked as duplicate.
4. hamilton/graph.py:243
- Draft comment:
The custom_style_function is checked to enforce that it only accepts keyword arguments. This validation is helpful, but it might also be useful to document the expected signature in the Sphinx docs so users know how to implement their own. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. hamilton/graph.py:393
- Draft comment:
In _get_node_style and _get_function_modifier_style, a series of if/elif clauses define style dictionaries. Consider refactoring to use a mapping/dictionary lookup so that adding new modifiers or node types can be done in a more DRY and declarative fashion. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. hamilton/graph.py:1096
- Draft comment:
The directional DFS traversal function creates and returns a tuple of sets (nodes and user_nodes). Consider adding more explicit type hints for parameters and return values to improve maintainability and readability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. hamilton/graph.py:1147
- Draft comment:
The execute method of FunctionGraph delegates to graph_functions.execute_subdag. Ensure that any exceptions or edge cases (e.g. cyclic dependencies) are properly handled, and consider adding unit tests for this functionality if not already present. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. pyproject.toml:29
- Draft comment:
The dependency specifications and optional-dependencies in pyproject.toml are extensive. It would be helpful to document in the Sphinx docs the purpose of various optional extras (e.g. 'dask', 'experiments', 'pandera', etc.) so that users know when to install which extras. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. pyproject.toml:161
- Draft comment:
Consider reviewing pinned dependency versions (e.g. in the 'dev' and 'docs' extras) to ensure compatibility with the broader ecosystem; if there are known issues, document them in the release notes or docs. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_Yjpdua4T2avAbOob
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
hamilton/graph.py
Outdated
| *functions: List[Tuple[str, Callable]], | ||
| config: Dict[str, Any], | ||
| adapter: lifecycle_base.LifecycleAdapterSet = None, | ||
| fg: Optional["FunctionGraph"] = None, | ||
| allow_module_overrides: bool = False, | ||
| allow_node_leveL_overrides: bool = False, |
There was a problem hiding this comment.
The parameter name 'allow_node_leveL_overrides' appears to have an inconsistent capitalization. Consider renaming it (e.g. to 'allow_node_overrides') for consistency with other parts of the code and to avoid confusion.
| def with_functions(self, *functions: FunctionType) -> "Builder": | ||
| """Adds the specified functions to the list. | ||
| This can be called multiple times. If you have allow_module_overrides | ||
| set this will enabl overwriting modules or previously added functions. | ||
|
|
||
| :param functions: | ||
| :return: self | ||
| """ | ||
| self.functions.extend(functions) | ||
| return self |
There was a problem hiding this comment.
I don't think we want this? Why can't we just put it in the .with_modules?
E.g. this opens up issues of precedence when using both modules and functions.
|
Some thoughts (no hard stance):
I'm a big fan of this change. I think it will make testing Hamilton easier, but also testing for end users |
elijahbenizzy
changed the title
Ok, to clarify, I'm pretty sure
But yeah, confusing. IMO, Also, FunctionGraph is not public facing, whereas driver is -- to check, does having it at the driver level unblock what you've been trying to do? |
|
From an internal slack discussions (thanks @zilto): Desired behavior:
|
elijahbenizzy
force-pushed
the
build-from-function-changes
branch
from
7d7fbfe to
229c3de
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 229c3de in 25 seconds
More details
- Looked at
243lines of code in4files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. pyproject.toml:29
- Draft comment:
Consider aligning version specifiers for dependencies consistently. For example, using an operator like '>=' might help prevent unexpected breakages later. - Reason this comment was not posted:
Confidence changes required:33%
None
2. pyproject.toml:60
- Draft comment:
The optional dependency for 'furo' is now unpinned. For stability, consider pinning or specifying a version range. - Reason this comment was not posted:
Confidence changes required:33%
None
3. pyproject.toml:68
- Draft comment:
Optional dependencies such as for Dask and others are defined well. Consider documenting in Sphinx docs how these extras are used. - Reason this comment was not posted:
Confidence changes required:50%
None
4. pyproject.toml:241
- Draft comment:
Dynamic versioning using hamilton.version looks good; ensure the attribute is maintained properly to avoid version mismatches. - Reason this comment was not posted:
Confidence changes required:0%
None
5. pyproject.toml:32
- Draft comment:
Consider replacing the version constraint for typing_extensions. Instead of 'typing_extensions > 4.0.0', using 'typing_extensions>=4.0.0' is a more conventional expression of minimum version requirements. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. pyproject.toml:40
- Draft comment:
Verify that the 'dask-core' optional dependency is available on PyPI as a standalone package. If not, consider revising this extra to avoid potential installation issues. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. pyproject.toml:60
- Draft comment:
Optional: Consider pinning versions for some documentation-related dependencies (e.g. 'furo') in the [project.optional-dependencies] 'docs' extra to improve reproducibility of builds. - Reason this comment was not posted:
Confidence changes required:50%
None
Workflow ID: wflow_s1B63DSbN9RtMQyp
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
function
This makes it so that we reduce code paths -- we'll eventually use
compile for everything. This also allows us to push functions up to the
user level, enabling `with_functions`
Note the following desired behavior:
Desired behavior:
- .with_modules() is the main entrypoint, users pass code that's stored in versionable file
- .allow_module_overrides() (current behavior) allows .with_modules(module_a, module_b) to have module_b override nodes from module_a
- .with_functions() allows to build a DAG from functions alone or in combination with .with_modules(). Node names must be unique (current behavior)
- .with_functions() + .allow_module_overrides() means the content from .with_functions() is applied last.
- nodes produced by .with_functions() are overriden in order if duplicates
elijahbenizzy
force-pushed
the
build-from-function-changes
branch
from
229c3de to
18442ee
Compare
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 18442ee in 1 minute and 22 seconds
More details
- Looked at
242lines of code in4files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. hamilton/graph.py:718
- Draft comment:
The static methods FunctionGraph.compile, from_modules and from_functions now all delegate to the same underlying implementation. Please add or update Sphinx documentation (e.g., in docs/api.rst or docs/graph.rst) to document these unified entry points and explain the intended internal API changes. - Reason this comment was not posted:
Comment was on unchanged code.
2. pyproject.toml:60
- Draft comment:
The dependency entry for 'furo' has been changed from a Git URL to a PyPI package. Please confirm that this matches your intended release strategy, as the Git ref might have served to get the latest preview features. - Reason this comment was not posted:
Comment did not seem useful.
3. hamilton/graph.py:64
- Draft comment:
Consider updating the error message in add_dependency to use function names (e.g. f.name) rather than printing raw function objects. This will help improve clarity when duplicate or incompatible types are detected. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. hamilton/graph.py:172
- Draft comment:
In compile_to_nodes, when a duplicate node is detected, the error message currently prints the conflicting function object. Consider formatting the message to use the function's name for better readability. - Reason this comment was not posted:
Comment was on unchanged code.
5. hamilton/graph.py:286
- Draft comment:
Typo in the docstring for _get_input_label: 'formatted aspyer a table' should be corrected. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. pyproject.toml:32
- Draft comment:
Consider specifying the version constraint for typing_extensions using '>=' instead of '>' for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_HYJDmwHMqckFv93y
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -142,17 +142,18 @@ def update_dependencies( | |||
| return nodes | |||
|
|
|||
|
|
|||
| def create_function_graph( | |||
| def compile_to_nodes( | |||
There was a problem hiding this comment.
The new parameter 'allow_node_level_overrides' and corresponding error message look good. Consider clarifying in the docstring that this option controls node‐level duplicate detection, as opposed to module overrides.
Buildng off the changes.
with_functionsin driver.compilefunction to replacefrom_functions,from_modulesTODO:
from_modulestocompilein the tests/other places it's used.from_modulesandfrom_functions(ensuring it's not used externally, it should be an internal API)