Skip to content
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

Review _should_include_node implementation, that changes test nodes tags #1479

Open
tatiana opened this issue Jan 21, 2025 · 0 comments
Open
Labels
area:selector Related to selector, like DAG selector, DBT selector, etc dbt:test Primarily related to dbt test command or functionality
Milestone

Comments

@tatiana
Copy link
Collaborator

tatiana commented Jan 21, 2025

For several releases, including 1.8.x, the method _should_include_node changes the nodes of type DbtResourceType.TEST to contain the parent node labels:

def _should_include_node(self, node_id: str, node: DbtNode) -> bool:
"""
Checks if a single node should be included. Only runs once per node with caching."""
logger.debug("Inspecting if the node <%s> should be included.", node_id)
if node_id in self.visited_nodes:
return node_id in self.selected_nodes
self.visited_nodes.add(node_id)
# Disclaimer: this method currently copies the tags from parent nodes to children nodes
# that are tests. This can lead to incorrect results in graph node selectors such as reported in
# https://github.com/astronomer/astronomer-cosmos/pull/1466
if node.resource_type == DbtResourceType.TEST and node.depends_on and len(node.depends_on) > 0:
node.tags = getattr(self.nodes.get(node.depends_on[0]), "tags", [])
logger.debug(
"The test node <%s> inherited these tags from the parent node <%s>: %s",
node_id,
node.depends_on[0],
node.tags,
)

While this was introduced in the past for some good reason - that we no longer remember - this can be very error-prone, as observed in #1466. The problem was very hard to troubleshoot, and - as the code is - we can still have other unexpected behaviours in Cosmos due to us injecting tags to nodes that didn't initially have them.

The goal of this ticket is:

  1. To understand why we are changing the test nodes
  2. If possible, modify this part of the code so it becomes less error-prone in the future
@tatiana tatiana added the area:selector Related to selector, like DAG selector, DBT selector, etc label Jan 21, 2025
@tatiana tatiana added this to the Cosmos 1.10.0 milestone Jan 21, 2025
@dosubot dosubot bot added the dbt:test Primarily related to dbt test command or functionality label Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:selector Related to selector, like DAG selector, DBT selector, etc dbt:test Primarily related to dbt test command or functionality
Projects
None yet
Development

No branches or pull requests

1 participant