-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Defer iff unselected reference does not exist in current env #2946
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -887,6 +887,7 @@ def resolve_doc( | |
|
||
def merge_from_artifact( | ||
self, | ||
adapter, | ||
other: 'WritableManifest', | ||
selected: AbstractSet[UniqueID], | ||
) -> None: | ||
|
@@ -898,10 +899,14 @@ def merge_from_artifact( | |
refables = set(NodeType.refable()) | ||
merged = set() | ||
for unique_id, node in other.nodes.items(): | ||
current = self.nodes[unique_id] | ||
if ( | ||
node.resource_type in refables and | ||
not node.is_ephemeral and | ||
unique_id not in selected | ||
unique_id not in selected and | ||
not adapter.get_relation( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you know if this happens before or after a relation cache has been built? Curious if this implementation is going to run one introspective query per node in the deferral manifest, or if they'll all be cache lookups? |
||
current.database, current.schema, current.identifier | ||
) | ||
): | ||
merged.add(unique_id) | ||
self.nodes[unique_id] = node.replace(deferred=True) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -372,7 +372,7 @@ def _get_deferred_manifest(self) -> Optional[WritableManifest]: | |
) | ||
return state.manifest | ||
|
||
def defer_to_manifest(self, selected_uids: AbstractSet[str]): | ||
def defer_to_manifest(self, adapter, selected_uids: AbstractSet[str]): | ||
deferred_manifest = self._get_deferred_manifest() | ||
if deferred_manifest is None: | ||
return | ||
|
@@ -382,17 +382,18 @@ def defer_to_manifest(self, selected_uids: AbstractSet[str]): | |
'manifest to defer from!' | ||
) | ||
self.manifest.merge_from_artifact( | ||
adapter=adapter, | ||
other=deferred_manifest, | ||
selected=selected_uids, | ||
) | ||
# TODO: is it wrong to write the manifest here? I think it's right... | ||
self.write_manifest() | ||
|
||
def before_run(self, adapter, selected_uids: AbstractSet[str]): | ||
self.defer_to_manifest(selected_uids) | ||
with adapter.connection_named('master'): | ||
self.create_schemas(adapter, selected_uids) | ||
self.populate_adapter_cache(adapter) | ||
self.defer_to_manifest(adapter, selected_uids) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok - i am seeing this now, and you can ignore my comment about caching above. Very clever, very cool. Leaving that comment above parce que je suis impressionné |
||
self.safe_run_hooks(adapter, RunHookType.Start, {}) | ||
|
||
def after_run(self, adapter, results): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a scenario where
unique_id
is present in the other manifest, but not in this manifest? I'm thinking that could happen if a model was deleted, for instance. If I'm thinking about that right, then I think this could raise a KeyError, and we'd probably want to useself.nodes.get(unique_id)
and do something smart if the result isNone
.I might be way off-base here, it just jumped out at me though and wanted to confirm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really good point, I was able to produce a
KeyError
here and changed the line to handle. this also gave me a clue to #2875 :)