updated to newest libs#21
Conversation
we call ref dump between dag commit and message handler.
a54c936 to
992684f
Compare
There was a problem hiding this comment.
Pull Request Overview
Updates the daggerml library to work with the newest version of daggerml-cli (0.0.37), adapting the internal API to handle changes in how DAG references are dumped and processed.
- Updated CLI dependency from version 0.0.33 to 0.0.37
- Modified DAG commit process to call
dml ref dump %before the message handler - Added test coverage for function caching functionality
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pyproject.toml | Bumped daggerml-cli dependency to version 0.0.37 |
| src/daggerml/core.py | Updated DAG commit logic to use new CLI API for reference dumping |
| tests/test_core.py | Added test for function caching with cache directory validation |
| submodules/daggerml_cli | Updated submodule commit to newer version |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -285,7 +285,7 @@ def make_node(dag: "Dag", ref: Ref) -> "Node": | |||
| class Dag: | |||
| _dml: Dml | |||
| _message_handler: Optional[Callable] = None | |||
There was a problem hiding this comment.
The type annotation change from Optional[Ref] to Optional[Union[Ref, Boxed]] indicates the field can now hold different types. This breaking change could confuse API consumers who expect a consistent type. Consider documenting this change or providing a migration guide.
| _message_handler: Optional[Callable] = None | |
| class Dag: | |
| """ | |
| Represents a DAG (Directed Acyclic Graph) for DaggerML operations. | |
| Migration Guide: | |
| ---------------- | |
| The type annotation for the `_ref` field has changed from `Optional[Ref]` to | |
| `Optional[Union[Ref, Boxed]]`. This means that `_ref` may now hold either a | |
| `Ref` instance or a `Boxed` instance. If you access `_ref` directly, you should | |
| check its type before using it: | |
| if isinstance(dag._ref, Boxed): | |
| value = dag._ref.value | |
| elif isinstance(dag._ref, Ref): | |
| # handle Ref as before | |
| ... | |
| elif dag._ref is None: | |
| # handle None case | |
| ... | |
| This change was made to support additional functionality. Please update your code | |
| accordingly if you rely on the `_ref` field. | |
| """ | |
| _dml: Dml | |
| _message_handler: Optional[Callable] = None | |
| # BREAKING CHANGE: The type annotation for `_ref` has changed from `Optional[Ref]` to | |
| # `Optional[Union[Ref, Boxed]]`. See the class docstring for migration guidance. |
| d0.result = nodes[0] | ||
| self.assertEqual(d0.result.value(), 3) | ||
| cache_list = dml("cache", "list", as_text=True) # response is jsonlines format | ||
| assert len([x for x in cache_list if x.rstrip() == "{"]) == 2 # this gets us unique maps |
There was a problem hiding this comment.
The assertion logic x.rstrip() == "{" is unclear and fragile. It appears to be counting JSON objects by looking for opening braces, but this could break if the JSON format changes. Consider parsing the JSON properly or using a more robust method to count cache entries.
| assert len([x for x in cache_list if x.rstrip() == "{"]) == 2 # this gets us unique maps | |
| # Count valid JSON objects in the jsonlines output | |
| assert len([x for x in cache_list if x.strip() and _is_json_object(x)]) == 2 |
We now have to call
dml ref dump %before the dag message handler. The api for the user is still the same.