Skip to content

Add first-class skipped node outcomes - #21

Open
magix022 wants to merge 2 commits into
mainfrom
issue-13-skipped-node-outcomes
Open

Add first-class skipped node outcomes#21
magix022 wants to merge 2 commits into
mainfrom
issue-13-skipped-node-outcomes

Conversation

@magix022

Copy link
Copy Markdown
Contributor

Summary

  • add ava.skip(reason, metadata=None) as a successful non-value node outcome
  • propagate authored skips consistently through local, Ray, operator/protobuf, and TUI paths
  • omit skipped fan-in slots, persistence writes, and lineage entries while keeping downstream dependencies complete
  • define rerun behavior and document the public contract

Verification

  • uv run ruff check src/ test/ — OK
  • uv run pytest test/skipped_outcome_test.py test/operator_tests/test_operator.py::TestOperatorLifecycle::test_authored_skip_is_recorded_and_run_succeeds test/tui_test.py::TestLogTimestamps::test_log_panel_renders_authored_skip_reason_and_metadata -q — 8 passed
  • uv run pytest test/dag_test.py test/workflow_execution_test.py test/workflow_data_passing_test.py test/rerun_test.py test/operator_tests/test_hooks.py test/operator_tests/test_operator.py test/operator_tests/test_grpc.py -m 'not ray and not tmux' -q — 160 passed, 28 deselected
  • uv run pytest test/workflow_execution_test.py test/workflow_data_passing_test.py test/operator_tests/test_hooks.py -m ray -q — 8 passed, 13 deselected
  • uv run pytest test/smoke_test.py test/example_smoke_test.py test/operator_example_discovery_test.py -v — 11 passed

Closes #13

@glesperance glesperance left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bottom line

REQUEST CHANGES — skip compaction is not shared by provider matching and Python binding, so a skipped branch can hide a live Stream input. Three other public-boundary inconsistencies make accepted skip outcomes non-durable or misbound. Reviewed exact head a2e1cf2c4c61ba5cc59626a75f78617bd413fff2 against merge base c1248186551a3d56b57c62ae14f1eb46397f9466.

Findings

  1. High — partial skipped fan-in selects the wrong Stream parent (src/avalanche/dag.py:1657, provider selection near :2671). Skip removal happens only during worker-side Python binding, after Stream has selected a positional parent from the unfiltered list. Reproduced with (skipped() & live_append()) >> consume(df=Stream(...)): the workflow returned None and bound Stream to ('skipped',) instead of the live AppendResult. Use one skip-compacted logical-slot plan for implicit Python and upstream-consuming providers, preserving worker-side Ray selection without driver payload fetches.

  2. High — accepted metadata can make a successful skip impossible to read (src/avalanche/types.py:30, src/runtime/operator/convert.py:96, src/runtime/executor.py:77). ava.skip() accepts arbitrary mapping values, but operator serialization later requires JSON. Reproduced: Path and bytes metadata are accepted, then node_state_to_proto raises TypeError; integer keys change on round trip, empty reason becomes absent, and unbounded metadata rides the small Ray status channel. Validate and recursively clone a bounded, strict JSON-safe schema at skip() with a nonblank bounded reason.

  3. Medium — skipped explicit positional-only inputs shift later values (src/avalanche/dag.py:1679). Removing the skipped element before call reconstruction compacts later explicit values into earlier slots. Independent audit reproduced consume(skipped(), live()) against consume(a="A", b="B", /) as ("LIVE", "B"). Preserve logical parameter identity, use the skipped slot's default when available, and reject skipped required positional-only slots clearly.

  4. Medium — per-slot multi-return skips silently lose skip status (src/avalanche/types.py:45). Reproduced: a num_returns=2 node returning (ava.skip("left missing"), "RIGHT") emits success and returns (None, "RIGHT"); the reason is not durable. Issue #13 defines a node-level outcome. Reject nested or per-slot SkipOutcome values for v1, or explicitly design slot-level status before accepting them.

Execution-service behavior

Skipped tasks still finalize and produce receipts; receipt fan-in remains separate from payloads, and Ray observes small status refs rather than task payloads. The focused skip/operator/TUI/execution-services suite passed 28/28, including real Ray. Preserve these properties while fixing the four boundaries above.

Merge and evidence state

The live PR was CONFLICTING; merge-tree conflicts were in CHANGELOG.md and src/tui/widgets/log_panel.py. Resolve against current main while preserving virtualized rendering and authored-skip display, then rerun focused TUI/operator tests. Missing regressions: partial/all-skip Stream fan-in under Local and real Ray, positional-only skips, per-slot skips, and hostile or oversized metadata.

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.

Add explicit skipped node outcomes

2 participants