Repository navigation
Add ADR for Dag importer process model - #73457
dilnazanlid wants to merge 2 commits into
Conversation
ephraimbuddy
left a comment
There was a problem hiding this comment.
Thanks for writing this down. The alternatives and explicit tradeoffs make the reasoning behind each decision easier to review.
I support giving importers control over their runtime. I’d like to clarify how that fits with an AIP I’m drafting on running Dag parsing through executors. It hasn’t reached the dev list yet, so please treat it as an early proposal.
Decision 1’s rejected alternative appears to rule out that direction, but I think it addresses a different design. In my proposal, executors would manage placement and capacity, while importers would retain control over the runtime inside each worker. My inline comments focus on making that boundary explicit and identifying the contracts the two proposals would need to agree on.
jason810496
left a comment
There was a problem hiding this comment.
Thanks a lot for drafting the design!
|
|
||
| ``` | ||
| ImportHandle what the manager does with it | ||
| is_ready ──────────────────► polled every loop pass — must never block |
There was a problem hiding this comment.
From my perspative, the is_ready should be renamed to is_finished. Since we're using "not proc.is_ready" as "finished".
airflow/airflow-core/src/airflow/dag_processing/manager.py
Lines 1375 to 1386 in b6538c3
airflow/airflow-core/src/airflow/dag_processing/processor.py
Lines 748 to 755 in b6538c3
There was a problem hiding this comment.
I considered both, but still is_ready seems more appropriate. It's the name the existing parse processes already use. DagFileProcessorProcess and LangSDKDagFileProcessorProcess both expose is_ready, and the manager's _collect_results loop polls exactly that property. ImportHandle is a structural protocol, so keeping the name means today's process classes already satisfy it without any renaming or adapter, and the manager loop doesn't change.
WDYT?
| ### 3. Fork mechanics and collector discipline belong to the base class | ||
|
|
||
| Forking out of a long-lived process carries a memory contract that is easy to miss: the child shares | ||
| the parent's pages until something writes to them, and a garbage collection writes to every object | ||
| it inspects. One collection in a freshly forked child therefore copies most of the parent's heap to | ||
| achieve nothing. | ||
|
|
||
| ``` | ||
| manager process parse child | ||
| ────────────────────────────────────────────── ──────────────────────── | ||
| prepare bundles | ||
| warm_importers() every importer realised, every warm_up() run | ||
| gc.collect() nothing collectable is left to be frozen | ||
| gc.freeze() the shared heap becomes permanent | ||
| │ | ||
| ├─ per import, steady state ───────────────── | ||
| │ fork_import(): gc.disable() |
There was a problem hiding this comment.
- section title: "Fork mechanics and collector discipline belong to the DagImporter.start_import"
- "long-lived process" -> "ephemeral processor subprocesses"
Additionally, should we show the hierarchy of manager -> start_import -- (processor subprocess) --> fork_import instead of just fork_import?
(Please note that there's another comment for suggestion better naming for the "start_import" method)
There was a problem hiding this comment.
Thanks, I added the call chain to the diagram, so it now shows submit_import() → fork_import() → spawn_child() → parse child.
I kept "long-lived" because there's no processor subprocess in between anymore. The importer runs inside the manager process, so the fork happens from the manager itself, which lives for the whole Dag processor run.
I also kept submit_import out of the title. The GC handling lives in fork_import, which is final, while submit_import is the method importers can override.
- Make submit_import non-blocking and fork by default, so importers written against import_definition never run in the manager; inline import becomes an opt-in for importers that run no author code. - Rename start_import/kill to submit_import/abort, drop pid, and record the naming rationale. - Add shut_down() to the importer lifecycle. - Make queue entries per definition, scope isolation to definitions, and resolve callbacks at their requested bundle version. - Have the parse child freeze its inherited heap before re-enabling the collector. - Fix the serialised result as the manager boundary and list its fields; keep callbacks Python-only for now.
ea155f1 to
0d07c1c
Compare
This PR introduces ADR-0001 to document the process model and supervision interfaces for AIP-85: Dag Importer.
Key decisions:
submit_import()and supervises the returnedImportHandle. By default the import forks a child (so existing importers keep working unchanged); importing inline is an opt-in for importers that run no author code; language runtimes can use a process they keep warm. This replaces the manager's current special-casing of coordinator importers.finalfork_import()keeps forked children from copying the manager's heap; importer authors never touch the collector.warm_up()/shut_down()hooks for long-lived state such as a warm runtime.DagFileParsingResultshape), so no Task SDK objects reach the manager and non-Python runtimes can produce it.The ADR is aligned with the native Lang-SDK Dag parsing (lang-sdk ADR-0010/0012, #74035).
Related to: AIP-85
Was generative AI tooling used to co-author this PR?