Embedding parents into models and snapshots

[ross-whatnot]

Following discussion in this issue and related draft PRs, taking a simpler approach of adding parents: list[Model | Source | Snapshot] to Model and Snapshot models

This should allow writing rules that compare a node to its parents ("any model with tag tier_1 may only have parents that also have tag tier_1", "any model that does not have the tag beta may not have any parents that do have the tag beta"), to be able to make assertions about model lineage expectation. One could also traverse the graph in full (upstream) with a recursive rule that walks the graph via parents.

Tested with a ~3000 model manifest locally, and things seem to work just fine; will do a bit more poking and prodding, though.

[jochemvandooren]

Amazing work @ross-whatnot , thanks a lot!

I have left very small comments, the feature itself is 🥇 . Could you add a CHANGELOG entry?

[ross-whatnot]

@jochemvandooren thanks! Think I've addressed everything.

[jochemvandooren]

🙌

[sercancicek]

Hello @ross-whatnot! Thank you for your contribution 🙌 The PR already looks very well. I left one comment/suggestion.

[sercancicek]

Suggested change

	def _populate_parents(self) -> None:
	"""Populate `parents` for all models and snapshots."""
	for node in list(self.models.values()) + list(self.snapshots.values()):
	for parent_id in node.depends_on.get("nodes", []):
	if parent_id in self.models:
	node.parents.append(self.models[parent_id])
	elif parent_id in self.snapshots:
	node.parents.append(self.snapshots[parent_id])
	elif parent_id in self.sources:
	node.parents.append(self.sources[parent_id])
	def _populate_parents(self) -> None:
	"""Populate `parents` for all models and snapshots."""
	all_parents = {**self.models, **self.snapshots, **self.sources}
	for node in list(self.models.values()) + list(self.snapshots.values()):
	for parent_id in node.depends_on.get("nodes", []):
	if parent := all_parents.get(parent_id):
	node.parents.append(parent)

IMO, this looks easier to read and maintain but just a suggestion :)

[matthieucan]

Tested with a ~3000 model manifest locally, and things seem to work just fine; will do a bit more poking and prodding, though.

Pretty curious about performances there: did you measure the delta?

[ross-whatnot]

Pretty curious about performances there: did you measure the delta?

@matthieucan no significant difference in the base case; for a project with 3000+ models, a simple lint takes basically the same amount of time (pre-parsed manifest).
Using 0.11:

Using my branch:

I haven't done anything too complex with the history, but adding a test that compares attributes of a child to attributes of its parents (e.g. below) doesn't change that runtime in any meaningful way (~1.11s)

@rule(severity=Severity.CRITICAL)
def model_has_appropriate_refs(model: Model) -> RuleViolation | None:
    """Models should have appropriate lineage"""
    tiers = ["t1", "t2", "t3", "t4"]
    violations = []
    for ref in model.parents:
        if isinstance(ref, Model):
            if tiers.index(model.meta["service_tier"]) < tiers.index(
                ref.meta["service_tier"]
            ):
                violations.append((ref.name, ref.meta["service_tier"]))
    if len(violations) > 0:
        return RuleViolation(
            message=f"Model {model.name} (tier {model.meta['service_tier']}) may not have parents in lower tiers ({', '.join([f'{v[0]} (tier {v[1]})' for v in violations])})"
        )
    return None

[matthieucan]

@ross-whatnot Awesome, thanks for sharing! 🎉

[jochemvandooren]

Amazing work @ross-whatnot , thank you 🙌

[jochemvandooren]

I will release a new version once we merge #110, so you can benefit from this feature asap!