fix: don't emit external models as multi-asset outputs (#65)#67
Open
Benrishty wants to merge 1 commit into
Open
fix: don't emit external models as multi-asset outputs (#65)#67Benrishty wants to merge 1 commit into
Benrishty wants to merge 1 commit into
Conversation
…server#65) `SQLMeshController.non_external_models_dag()` did not actually filter external models — its only guard was `if not model`, which skips FQNs that resolve to no model at all. External models resolve to a real `Model` (kind.is_external), so they passed through and `to_asset_outs()` emitted an `AssetOut` for each. External models are source tables the project READS, so materializing them as outputs is wrong and collides with any existing asset that already owns that key (e.g. a dlt asset), which surfaces as `DagsterInvalidDefinitionError: Duplicate asset key` (see opensource-observer#65, opensource-observer#53). Two changes make externals dependency-only: 1. `non_external_models_dag()` now skips `model.kind.is_external`, so externals are no longer emitted as outs (and drop out of `resource.py`'s materializable-model set, which is also correct). 2. `to_asset_outs()`'s dependency loop now routes external-model deps to the asset-dep path. This is required: an external dep resolves to a truthy `dep.model`, so with only change (1) it would land in `internal_asset_deps` without a matching out OR dep — which Dagster rejects ("specified as asset inputs, but not in internal_asset_deps"). Routing it through `create_asset_dep` keeps deps and internal_asset_deps consistent. Backward compatible: projects with no external models are unaffected (the sample project has none, so its existing to_asset_outs test is unchanged). Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #65 (and the root cause behind #53).
Problem
SQLMeshController.non_external_models_dag()doesn't actually filter external models. Its only guard isif not model: continue, which skips FQNs that resolve to no model — but an external model resolves to a realModel(kind.is_external == True), so it passes through, andto_asset_outs()builds anAssetOutfor it. External models are source tables the project reads, so emitting them as outputs is wrong: it materializes phantom assets and collides with any existing asset that already owns that key (e.g. adltasset landing the same physical table) →DagsterInvalidDefinitionError: Duplicate asset key.This is what #65 observed (correctly spotting
non_external_models_dag) and the symptom #53 hit.Fix
Two small changes so externals become dependency-only edges:
controller/base.py—non_external_models_dag()now skipsmodel.kind.is_external(living up to its name). Externals are no longer emitted as outs, and also drop out of the materializable-model setresource.pyderives from it.controller/dagster.py—to_asset_outs()'s dependency loop routes external-model deps down the asset-dep path. This is required, not cosmetic: an external dep resolves to a truthydep.model, so with change (1) alone it would be added tointernal_asset_depswithout a matching out or dep, and Dagster rejects that (... specified as asset inputs, but are not specified in internal_asset_deps). Routing it throughcreate_asset_depkeepsdepsandinternal_asset_depsconsistent.Verification
Tested against a real project with 5 external models (4 source tables + 1 cross-database dimension):
Duplicate asset key.deps, never inouts; the multi-asset builds cleanly; the remapped-external case no longer collides.Backward compatible: projects with no external models are unaffected — including this repo's
sample/sqlmesh_project, so the existingtest_sqlmesh_context_to_asset_outsassertions (deps == 1,outs == 10) are unchanged.Note on tests
The sample project currently has no external model, which is why this path wasn't caught. I'm happy to add a regression test, but it needs a fixture with an external model (either extending the sample — which would shift the counts in the existing test — or a dedicated fixture). Let me know which you'd prefer and I'll push it in this PR.