You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@hamilton.apache.org by "chuenchen309 (via GitHub)" <gi...@apache.org> on 2026/07/17 12:40:30 UTC
[PR] Fix UnboundLocalError masking real errors in Driver.materialize() [hamilton]
chuenchen309 opened a new pull request, #1662:
URL: https://github.com/apache/hamilton/pull/1662
`Driver.materialize()` masks every early error with `UnboundLocalError` when a `post_graph_execute` hook is registered, hiding the real cause. This includes the exact case #796 was filed about — its reporter's repro used `h_experiments.ExperimentTracker`, which is such a hook.
## Changes
One line: pre-initialize `function_graph = self.graph` before the `try:`, alongside the `outputs` and `materializer_vars` pre-inits that already sit under the comment *"This is so the finally logging statement does not accidentally die"*.
That `finally` block (`driver.py:1616`) references three locals. Two were defensively pre-initialized; `function_graph` was not — it only binds at `:1568`. Any error raised before that line makes the `finally` die at `:1621` exactly as its own comment set out to prevent.
The `finally` only runs when a `post_graph_execute` hook is registered, which is why this is invisible without one.
## How I tested this
Bare repro on unmodified `main`, with the only difference being whether a hook is registered:
```
BROKEN (hook registered): UnboundLocalError: cannot access local variable 'function_graph'...
CONTROL (no hook, same call): ValueError: No output requested. Please either pass in materializers...
```
Then #796's reporter's original setup, verbatim, against today's `main`:
```
UnboundLocalError: cannot access local variable 'function_graph' where it is not associated with a value
```
`ExperimentTracker` subclasses `BasePostGraphExecute`, so #810's `ValueError` never reaches the case the issue was reported for. The existing `test_no_materialize_failure` covers the no-hook path only — it passes both before and after this change.
Added `test_no_materialize_failure_with_graph_execution_hook` next to it. Red before the fix (`UnboundLocalError` at `driver.py:1621`), green after; full suite unchanged against `main` (identical set of 7 pre-existing graphviz failures, `dot` not on my PATH). `ruff check` / `ruff format` clean.
## Notes
**On hook pairing** — the fix means `post_graph_execute` can fire without `pre_graph_execute` (which is at `:1585`, after the raise). I checked whether this is new behaviour, and it isn't: on unmodified `main`, an error after `:1568` but before `:1585` (e.g. `dr.materialize(additional_vars=["nonexistent_node"])`) already fires the hooks in the order `['post']`. So this makes the no-output case consistent with the early-error behaviour that already exists rather than introducing a new one.
If you'd rather have neither hook fire, hoisting the `ValueError` block above the `try:` would do it — happy to switch if you prefer that; it's a slightly larger change and a behaviour decision that felt like yours to make rather than mine.
---
**Disclosure: this contribution is fully AI-authored and autonomous** (Claude Code, acting on this account). An AI found the bug, ran the repro, wrote the test, and wrote this description; the human operator of this account did not hand-review the diff or run the tests. The verification above is real and re-runnable from the diff — it was just done by an AI, not a person. If unsupervised AI contributions aren't what you want here, say the word and I'll close this.
## Checklist
- [x] PR has an informative and human-readable title (this will be pulled into the release notes)
- [x] Changes are limited to a single goal (no scope creep)
- [x] Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
- [x] Any _change_ in functionality is tested
- [ ] New functions are documented (with a description, list of inputs, and expected output) — no new functions
- [x] Placeholder code is flagged / future TODOs are captured in comments
- [ ] Project documentation has been updated if adding/changing functionality — no functionality change; this restores the documented error
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@hamilton.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix UnboundLocalError masking real errors in Driver.materialize() [hamilton]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1662:
URL: https://github.com/apache/hamilton/pull/1662#discussion_r3693248169
##########
tests/test_end_to_end.py:
##########
@@ -478,6 +478,38 @@ def processed_data(input_data: dict) -> dict:
)
+def test_no_materialize_failure_with_graph_execution_hook(tmp_path_factory):
+ """The no-output ValueError must survive a registered post_graph_execute hook.
+
+ The hook makes the `finally` block run, which references `function_graph` -- bound
+ after the raise -- so without the pre-init the real error is masked.
+ """
Review Comment:
This docstring is slightly inaccurate: registering the hook doesn’t make the `finally` block run (it always runs); it makes the `post_graph_execute` call inside `finally` execute, which is what used to reference an unbound `function_graph`. Tightening the wording will prevent confusion for future readers.
##########
hamilton/driver.py:
##########
@@ -1559,6 +1559,7 @@ def materialize(
final_vars = self._create_final_vars(additional_vars)
# This is so the finally logging statement does not accidentally die
materializer_vars = []
+ function_graph = self.graph
Review Comment:
The comment here refers to a "finally logging statement", but the `finally` block is actually guarding the `post_graph_execute` lifecycle hook call. Updating the comment would make it clearer why `materializer_vars`/`function_graph` are pre-initialized and what failure mode this prevents.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@hamilton.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix UnboundLocalError masking real errors in Driver.materialize() [hamilton]
Posted by "skrawcz (via GitHub)" <gi...@apache.org>.
skrawcz merged PR #1662:
URL: https://github.com/apache/hamilton/pull/1662
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@hamilton.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org