Repository navigation
Add configuration support for custom DAG importers - #72442
dilnazanlid wants to merge 4 commits into
Conversation
16d9180 to
936de37
Compare
SameerMesiah97
left a comment
SameerMesiah97
left a comment
There was a problem hiding this comment.
Not an expert here but this looks fairly clean from my perspective but we need to wait for CI to be triggered as there is a high risk of breaking changes here. I have left some comments.
There was a problem hiding this comment.
Nice! Thanks for the PR, would it be better to encapsulate the logic for handling different level of importer configs into the DagImporterRegistry?
IIUC, the lifecycle of the DagImporterRegistry should be similar as the CoordinatorManager, so I prefer to leverage the @functools.cache to ensure the singleton and mirror the lazy import for those actual importer class.
| def get_importer_registry(self, bundle_name: str) -> DagImporterRegistry: | ||
| """Get the DAG importer registry for a bundle.""" | ||
| if bundle_name not in self._bundle_importers: | ||
| cfg = self._bundle_config.get(bundle_name) | ||
| importers_config = cfg.importers if cfg else None | ||
| self._bundle_importers[bundle_name] = self.create_importer_registry(bundle_name, importers_config) | ||
| return self._bundle_importers[bundle_name] |
There was a problem hiding this comment.
IIUC, the purpose of coupling DagImporterRegistry and DagBundlesManager is re-using the _bundle_config.
Would it be better to introduce a common layer for retrieving the configs like -- https://github.com/apache/airflow/pull/70805/changes#diff-1232041046ad2f8635ae867e2cf30914e6ad4619401897d723f6e57731c05e7bR118-R136.
Or wait until #70805 first, then make the _load_bundle_config_snapshot a public function then we don't need to couple DagImporterRegistry and DagBundlesManager together.
There was a problem hiding this comment.
Correct, it was for the _bundle_config and parsing of the importer classes in the bundle manager. I added the similar code as in #70805 in here too, to check it works correctly. Overall, it would be good to submit this(and other 2 PRs) because there is last PR that will need all and use importers from SDK and move the filesystem path to dag definition.
Do you think your change is going to be submitted soon?
6cec340 to
fdfb6da
Compare
dilnazanlid
changed the title
…d extensions reach parsing instead of being ignored by hardcoded defaults.
Avoid premature class loading and tight coupling between bundles and importer registries by delegating 3-tier configuration resolution to DagImporterRegistry with cached lazy instantiation.
db3a2a0 to
1015e86
Compare
1015e86 to
db3a2a0
Compare
|
Closing this in favour of a single change where we replace the This PR introduces the So landing the config here means (a) building plumbing in a package that the very The follow-up PR will instead do it in one step:
All the discussed points here were already accommodated in the sdk/importers in #72369. Thanks! |
github-project-automation
Bot
moved this from In Progress
to Done
in AIP-85 Dag Importer
3 participants
This pull request was one of the incremental parts of the AIP-85: DAG Importer implementation. See closing comment.
Description
This pull request is part of the implementation of AIP-85: DAG Importer. It introduces configuration-driven registration and resolution for custom Dag importers at both the global and Dag bundle levels.
Prior to this change, Dag file discovery and parsing were tied to built-in Python file discovery (
list_py_file_paths), and theDagImporterRegistryoperated strictly as a singleton with hardcoded defaults. This PR introduces a configurable, tiered Dag importer resolution mechanism:Shared Module Loading (
airflow_shared.module_loading.dag_importers):load_dag_importers()utility to dynamically instantiate importer classes viaclasspathandkwargs.extensionsconfiguration per importer, falling back to the importer'ssupported_extensionsattribute or method.AirflowConfigExceptionon malformed inputs.3-Tier Importer Registry Precedence in Dag Bundles (
DagBundlesManager):_ExternalBundleConfig,_InternalBundleConfig) to support an optionalimporterslist.DagImporterRegistry:bundle.importers): Importers explicitly declared for that specific bundle (highest priority).[dag_processor] dag_importer_configs): System-wide custom importers configured via Airflow config.PythonDagImporterhandling.pyand.zipfiles (baseline fallback).bundle.importer_registryproperty toBaseDagBundleand cached per-bundle registries inDagBundlesManager.Instantiable
DagImporterRegistry:DagImporterRegistryfrom a rigid process-wide singleton into an instantiable class, allowing isolated registries per Dag bundle while preservingget_importer_registry()for global access.register()to allow custom extension lists and log informative warnings when an extension mapping is overridden by a higher-priority importer.Integration with File Discovery and
DagBag:DagFileProcessorManager._find_files_in_bundle()to discover Dag files dynamically usingbundle.importer_registry.list_dag_files().DagBagto accept and utilize bundle-scopedimporter_registryinstances during Dag parsing and collection.Tests:
airflow_shared.module_loading.test_dag_bundle_manager.pyverifying the 3-tier precedence rules, extension overrides, nested composite importers, and error handling.test_registry.pyandtest_manager.pyto cover independent registry instances and bundle discovery.Was generative AI tooling used to co-author this PR?
Generated-by: Google Gemini following the guidelines