feat: execute graph-derived artifacts through generic dispatch - #22
Conversation
Loading a bundle entry point through the standard source loader wrote a __pycache__ into the bundle, so the producer's next verification (the MotionKernel finalize stage) rejected the bundle as holding undeclared files and the campaign failed after a successful validation run. The same loader also preferred any valid cached .pyc over the verified source and reopened the mutable path after hashing it. Execute the entry point from an immutable byte snapshot bound to the digest the verifier accepted, with bytecode writes suppressed; compare the full re-verified manifest against the validated one; enumerate symlinks during the undeclared-file sweep; and stop ignoring __pycache__, matching the producer's validator.
A raise between hook attachment and the profiler's try block orphaned the forward hooks for the rest of the run. Also carry the full capture schema in the finalize-failure record, and coalesce no-tensor-input graph breaks per scope while they are recorded instead of appending one entry per call.
…ent/v1-subgraph-dispatch
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| except AssertionError: | ||
| observed_context = None | ||
| variant = _ShapeVariant( | ||
| inputs=inputs, | ||
| outputs=outputs, | ||
| example_args=args, | ||
| example_kwargs=dict(kwargs), | ||
| observed_context=observed_context, | ||
| calls=1, | ||
| ) |
There was a problem hiding this comment.
capture_export_invocation only catches AssertionError from get_forward_context(), but the same except AssertionError in _update_scope was widened to except Exception in this very PR. When get_forward_context() raises a non-AssertionError (e.g. RuntimeError or ImportError), the exception escapes the local try-block and is caught by the outer except Exception in _decide, which misclassifies the scope as "identity unavailable" and logs a misleading reason rather than gracefully setting observed_context = None.
| except AssertionError: | |
| observed_context = None | |
| variant = _ShapeVariant( | |
| inputs=inputs, | |
| outputs=outputs, | |
| example_args=args, | |
| example_kwargs=dict(kwargs), | |
| observed_context=observed_context, | |
| calls=1, | |
| ) | |
| except Exception: # noqa: BLE001 - optional metadata only | |
| observed_context = None | |
| variant = _ShapeVariant( | |
| inputs=inputs, | |
| outputs=outputs, | |
| example_args=args, | |
| example_kwargs=dict(kwargs), | |
| observed_context=observed_context, | |
| calls=1, | |
| ) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: fastvideo/optimization/fx_capture.py
Line: 1182-1191
Comment:
`capture_export_invocation` only catches `AssertionError` from `get_forward_context()`, but the same `except AssertionError` in `_update_scope` was widened to `except Exception` in this very PR. When `get_forward_context()` raises a non-`AssertionError` (e.g. `RuntimeError` or `ImportError`), the exception escapes the local try-block and is caught by the outer `except Exception` in `_decide`, which misclassifies the scope as "identity unavailable" and logs a misleading reason rather than gracefully setting `observed_context = None`.
```suggestion
except Exception: # noqa: BLE001 - optional metadata only
observed_context = None
variant = _ShapeVariant(
inputs=inputs,
outputs=outputs,
example_args=args,
example_kwargs=dict(kwargs),
observed_context=observed_context,
calls=1,
)
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| previous_dont_write_bytecode = sys.dont_write_bytecode | ||
| sys.dont_write_bytecode = True | ||
| try: | ||
| spec.loader.exec_module(module) | ||
| code = compile(source, str(entry_file), "exec", dont_inherit=True) | ||
| exec(code, module.__dict__) # noqa: S102 - verified artifact is executable | ||
| except Exception as exc: # noqa: BLE001 - untrusted code, any failure is a rejection | ||
| sys.modules.pop(module_name, None) | ||
| raise _fail( | ||
| str(directory), | ||
| "entry_point", | ||
| f"importing {verified.entry_file!r} raised {type(exc).__name__}", | ||
| ) from exc | ||
| finally: | ||
| sys.dont_write_bytecode = previous_dont_write_bytecode |
There was a problem hiding this comment.
sys.dont_write_bytecode save/restore is not thread-safe. Two concurrent load_entry_point calls can race: Thread B reads sys.dont_write_bytecode after Thread A has already set it to True, so Thread B saves True as its previous_dont_write_bytecode. When Thread A finishes and restores False, Thread B's finally then restores True, permanently flipping the interpreter flag. Since compile+exec does not invoke the import machinery itself, consider using a threading lock or dropping the flag entirely.
Prompt To Fix With AI
This is a comment left during a code review.
Path: fastvideo/optimization/artifact.py
Line: 778-791
Comment:
**`sys.dont_write_bytecode` save/restore is not thread-safe.** Two concurrent `load_entry_point` calls can race: Thread B reads `sys.dont_write_bytecode` after Thread A has already set it to `True`, so Thread B saves `True` as its `previous_dont_write_bytecode`. When Thread A finishes and restores `False`, Thread B's `finally` then restores `True`, permanently flipping the interpreter flag. Since `compile`+`exec` does not invoke the import machinery itself, consider using a threading lock or dropping the flag entirely.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| try: | ||
| try: | ||
| unshard(async_op=False) | ||
| except TypeError: | ||
| unshard() | ||
| yield | ||
| finally: | ||
| reshard() |
There was a problem hiding this comment.
reshard() is called unconditionally even when unshard() failed. If unshard(async_op=False) raises TypeError and the no-arg fallback unshard() also raises, the outer try…finally still calls reshard() on a module that was never unsharded. For some FSDP2 implementations this could corrupt internal state or shadow the original failure. A guard flag set to True only after a successful unshard() call, checked before reshard(), would prevent this.
Prompt To Fix With AI
This is a comment left during a code review.
Path: fastvideo/optimization/dispatch.py
Line: 349-356
Comment:
**`reshard()` is called unconditionally even when `unshard()` failed.** If `unshard(async_op=False)` raises `TypeError` and the no-arg fallback `unshard()` also raises, the outer `try…finally` still calls `reshard()` on a module that was never unsharded. For some FSDP2 implementations this could corrupt internal state or shadow the original failure. A guard flag set to `True` only after a successful `unshard()` call, checked before `reshard()`, would prevent this.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
1 participant
Completes the generic FastVideo V1 consumer on top of executable graph IR: safe subgraph rewrite, live parameter materialization, managed-hook lifecycle dispatch, immutable hash-verified artifact loading, structured fallbacks, and profiler cleanup hardening. No model-specific dispatch branch is introduced.\n\nValidation: all 73 FastVideo optimization tests passed. Wan GB200 job 967 loaded one verified artifact, dispatched 719/719 transformer-block calls with zero runtime fallbacks, and produced byte-equal output frames.
Greptile Summary
This PR completes the generic FastVideo V1 dispatch consumer over executable graph IR, wiring together subgraph rewrite dispatch, FSDP2 parameter materialization, module-hook lifecycle integration, and immutable hash-verified artifact loading. It introduces a new
subgraph.pyrewrite engine and tightens the security ofload_entry_pointby switching fromimportlibtocompile+execwith a full manifest integrity check.subgraph.py(new):rewrite_exported_subgraphbuilds a per-blockfx.GraphModulewhose lifted attributes resolve against the live repeated block, replacing the declared subgraph region with a singlecall_functionto the trusted artifact; aWeakKeyDictionarycache avoids rebuilding for every call.dispatch.py: adds_materialized_candidate_parameters(mirrors FSDP2unshard/reshardlifecycle),_parameter_managerancestry walk, and routes hook-managed modules throughModuleHookManager.run_with_forwardduring candidate dispatch.artifact.py: replaces theartifact_id-only post-load check with a fullreplace(verified, directory=\u2026) != manifestequality guard and switches the module loader tocompile+execwith a re-verified byte digest, preventing a path-swap between validation and import.Confidence Score: 4/5
Safe to merge with minor follow-up fixes; all three findings are on error or diagnostic paths and do not affect correct-case execution.
The core dispatch, subgraph rewrite, and artifact-loading changes are well-structured and comprehensively tested. All three flagged items live on error or ancillary paths: the forward-context exception narrowing can only misclassify a diagnostic field; the sys.dont_write_bytecode race is cosmetic given that compile+exec does not write .pyc files anyway; and the unconditional reshard() call only matters when unshard() itself fails, which is not a normal operating condition.
Files Needing Attention: fastvideo/optimization/dispatch.py (_materialized_candidate_parameters unshard/reshard guard), fastvideo/optimization/artifact.py (sys.dont_write_bytecode thread safety), fastvideo/optimization/fx_capture.py (capture_export_invocation exception scope)
Important Files Changed
reshard()is called unconditionally even whenunshard()failedsys.dont_write_bytecodesave/restore is not thread-safe; new subgraph manifest fields validated correctlycapture_export_invocation, stabilises op-key via schema name, adds stride/device_type to IR tensor meta, and fixesno_tensor_inputsover-counting;AssertionErrorcatch for forward context is inconsistently narrower than the fix in_update_scoperun_with_forwardfrom the inline wrapper closure, enabling dispatch to route candidate calls through the module hook lifecycle without installing additional hooksfinalizeis called even when a pre-profiler step failsFASTVIDEO_OPTIMIZATION_ARTIFACT_VALIDATIONenv var; correctly parsed as bool, consistent with other boolean env varsFASTVIDEO_OPTIMIZATION_ARTIFACT_VALIDATIONenv var; accurate descriptionSequence Diagram
sequenceDiagram participant Caller participant GraphDispatchSession participant ModuleHookManager participant FSDP2Module participant Artifact Caller->>GraphDispatchSession: _dispatch(wrapper, args, kwargs) GraphDispatchSession->>GraphDispatchSession: check _decisions cache alt First call for (scope, shape_key) GraphDispatchSession->>GraphDispatchSession: native_forward(args, kwargs) GraphDispatchSession->>GraphDispatchSession: _decide() - select artifact Note over GraphDispatchSession: identity captured, artifact loaded via compile+exec else Subsequent calls GraphDispatchSession->>FSDP2Module: "unshard(async_op=False)" alt hook_manager present GraphDispatchSession->>ModuleHookManager: run_with_forward(candidate_forward, args) ModuleHookManager->>Artifact: "candidate_forward(*args)" Artifact-->>ModuleHookManager: output ModuleHookManager-->>GraphDispatchSession: output else no hook_manager GraphDispatchSession->>Artifact: "candidate_forward(*args)" Artifact-->>GraphDispatchSession: output end GraphDispatchSession->>FSDP2Module: reshard() end GraphDispatchSession-->>Caller: resultPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'fork/agent..." | Re-trigger Greptile