Skip to content

Fix apply_op for tuple containers and keyword container inputs - #53

Open
Max Freedom Pollard (MaxFreedomPollard) wants to merge 1 commit into
microsoft:mainfrom
MaxFreedomPollard:fix/apply-op-tuple-and-kwargs
Open

Fix apply_op for tuple containers and keyword container inputs#53
Max Freedom Pollard (MaxFreedomPollard) wants to merge 1 commit into
microsoft:mainfrom
MaxFreedomPollard:fix/apply-op-tuple-and-kwargs

Conversation

@MaxFreedomPollard

@MaxFreedomPollard Max Freedom Pollard (MaxFreedomPollard) commented Sep 4, 2026

Copy link
Copy Markdown

Summary

apply_op (exported from opto.trace) broadcasts an operator over a container of Nodes. The Broadcasting section of docs/tutorials/basic_tutorial.ipynb documents the supported containers as list, tuple, dict, or a container class — but a tuple has never worked, and neither have container inputs passed by keyword. The tutorial only demonstrates the list, dict and class cases, so the gap isn't visible from the notebook output.

The source change is +6/-2 in opto/trace/broadcast.py; the rest is regression tests.

1. Tuple containers raise TypeError

The list/tuple branch assigns into output element by element, which a tuple cannot do:

from opto.trace import node, apply_op
import opto.trace.operators as ops

apply_op(ops.add, (node("x1"), node("x2")),
                  (node("a1"), node("a2")),
                  (node("b1"), node("b2")))
TypeError: 'tuple' object does not support item assignment

The intent was already in the code — the branch ends with

if isinstance(output, tuple):
    output = tuple(output)

which is a no-op as written, and for a non-empty tuple the loop raises before ever reaching it. (An empty tuple does reach it, and round-trips unchanged.) The fix records whether the output was a tuple, works on a list, and converts back at the end. Lists keep their current in-place behaviour, which test_apply_op.py relies on.

2. Keyword container inputs hit an assertion

The NodeContainer branch builds _kwargs with

kk: vv if isinstance(v, Node) else getattr(vv, k)

v is the output's attribute; vv is the keyword input. When an attribute of the output is a Node, every keyword input is forwarded whole instead of being indexed by attribute name, and the recursive call trips the admissible-type assertion:

from opto.trace import node, bundle, apply_op
from opto.trace.containers import NodeContainer

class C(NodeContainer):
    def __init__(self, x):
        self.x = node(x)

@bundle()
def concat(foo, bar):
    return foo + bar

apply_op(concat, C("seed"), foo=C("foo"), bar=C("bar"))
AssertionError

The positional path one line above already uses the input (x), and the list and dict branches already test isinstance(vv, Node). This makes the keyword path consistent with them; the same call passed positionally works today and returns "foobar".

Tests

tests/unit_tests/test_apply_op.py gains coverage for:

  • a tuple container standalone, and nested in a NodeContainer, a list, and a dict — checking the result type, the values, and that the inputs are wired in as parents
  • a bare Node broadcast against every element of a tuple
  • a list container still being updated in place
  • keyword inputs whose values are containers, and keyword inputs mixing a container with a bare Node

Both new blocks were re-run against the unpatched broadcast.py: the tuple cases fail with the TypeError above, the keyword cases with the AssertionError above. Both pass with the fix.

Verification

The Python unit tests workflow in this repo is awaiting maintainer approval here, as it does for any first-time contributor. I ran that same workflow, unmodified, on this exact commit in my fork:

https://github.com/MaxFreedomPollard/Trace/actions/runs/33940060529 — green, 22/22 (test_apply_op.py included), Python 3.9.25 on ubuntu-24.04.

Also checked locally:

result
python tests/unit_tests/run.py on Python 3.9 22/22, exit 0
python tests/unit_tests/run.py on Python 3.12 22/22, exit 0
black==23.3.0 (pinned in .pre-commit-config.yaml) both files unchanged
ruff==0.0.261 (pinned) no new findings
codespell==2.2.6 (pinned) clean

No API, signature, or dependency changes. The one pre-existing E501 in broadcast.py (line 57, a commented-out line) is untouched.

@MaxFreedomPollard

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

apply_op broadcasts an operator over containers of Nodes, but two cases
in opto/trace/broadcast.py never worked:

- A tuple container raised "TypeError: 'tuple' object does not support
  item assignment", because the loop assigned into the tuple in place.
  The trailing `if isinstance(output, tuple): output = tuple(output)`
  was therefore unreachable. Accumulate into a list and convert back.

- Keyword inputs against a Node-valued attribute of a NodeContainer
  tested `isinstance(v, Node)` (the output's attribute) instead of
  `isinstance(vv, Node)` (the keyword input), so a container passed by
  keyword was forwarded whole instead of being indexed by attribute,
  tripping the admissible-type assertion. The positional path already
  did the right thing.

Adds regression tests covering a tuple container standalone and nested
in a NodeContainer, a list and a dict; a bare Node broadcast against a
tuple; the existing in-place list behaviour; and keyword inputs whose
values are containers, including one mixed with a bare Node.
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.

1 participant