Looking for feedback: adds interceptor lifecycle hook#602
Looking for feedback: adds interceptor lifecycle hook#602
Conversation
|
A preview of 105da95 is uploaded and can be seen here: ✨ https://burr.dagworks.io/pull/602 ✨ Changes may take a few minutes to propagate. Since this is a preview of production, content with |
|
@skrawcz Thanks for opening this — I really like the direction of making remote execution explicit via lifecycle hooks. Before diving into design feedback, I’m trying to fully understand the current execution flow vs the new interception path. A few things I’m exploring:
I’ll follow up with more concrete observations once I’ve traced the execution path locally. |
|
Hey @skrawcz Running the full test suite locally shows failures related to missing optional integration dependencies (redis, mongodb, asyncpg, langchain, etc.), not interceptor behavior. From what I can see so far, the interceptor lifecycle implementation itself appears stable under 3.11. I’m continuing to look into what might be causing the CI 3.11/3.12 failures — especially around async execution paths or example validation. |
|
@skrawcz I ran the full core test suite locally under Python 3.11. All execution, lifecycle, async, and persistence tests pass (264 passing). The only failure is in test_graphviz_display, which requires the Graphviz system binary (dot) to be installed and available on PATH. This appears to be an environment/dependency issue rather than a behavioral regression in the interceptor or lifecycle implementation. Given that all core execution paths pass under 3.11, I don’t see evidence of a Python-version-specific async or hook-related issue. If CI failures are similar, it may be worth ensuring the Graphviz binary is installed in the CI environment or marking visualization tests as optional. |
|
Hey @skrawcz — quick update from my side: I ran core tests locally on Python 3.11 and they pass (264 passed; only optional Graphviz dot env issue can fail if binary missing). CI is currently failing for py3.11/py3.12 + validate-examples, but the logs expired, so we can’t see root cause. Could you please re-run the failed jobs (or the full workflow) so we get fresh logs? Once logs are available, I’ll triage and propose a fix PR (workflow/env vs code). |
|
@skrawcz — local test update (Windows, Python 3.12):
I did notice one PytestUnraisableExceptionWarning related to SQLitePersister.del, but it does not fail the suite. If there are specific CI failures you’d like me to focus on, I’m happy to investigate further. |
|
@goutamk09 sorry missed all these. will try to find some time this week to respond -- feel free to use the dev@ apache email list next time to discuss ... |
|
Thanks @skrawcz — no worries at all. I’ll use the dev@ list next time for broader discussion. |
This proposes a new hook to allow one to remotely push execution of a Burr Action. For example, if one wants to selectively push remote execution of an action to say Ray, then one can now build an interceptor. This also introduces companion hooks that would run pre & post on the remote to mirror what Burr has today - -I thought it would be simpler to not try to reuse those hooks, and instead make it explicit as to what is going on if you wanted to add something similar. See tests for examples, but open to feedback here. Note: we'd need to think through how this plays with parallelism functionality we have. E.g. currently interceptors aren't propagated to sub applications...
These examples needed to be vetted more. If the interceptors here seem good, we could look to bring them in first class to the library.
4f64587 to
105da95
Compare
This proposes a new hook to allow one to remotely push execution of a Burr Action.
For example, if one wants to selectively push remote execution of an action to say Ray,
then one can now build an interceptor.
This also introduces companion hooks that would run pre & post on the remote to mirror
what Burr has today - -I thought it would be simpler to not try to reuse those hooks, and
instead make it explicit as to what is going on if you wanted to add something similar.
See tests for examples, but open to feedback here.
Note: we'd need to think through how this plays with parallelism functionality we have.
E.g. currently interceptors aren't propagated to sub applications...
Changes
How I tested this
Notes
This change needs to have some thought and discussion.
Checklist