Add default source nodes rendering

[arojasb3]

Description

Re-Opening of PR #661
This PR features a new way of rendering source nodes:

• Check freshness for sources with freshness checks
• Source tests
• Empty operators for nodes without tests or freshness.

One of the main limitations I found while using the custom_callback functions on source nodes to check freshness is that nodes were being created on 100% of sources but not all of them required freshness checks, this made workers waste compute time.

I'm adding a new variable into the DbtNode class called has_freshness which would be True for sources with freshness checks and False for any other resource type.

If this feature is enabled with the option ALL:
All sources with the has_freshness == False will be rendered as Empty Operators, to keep the dbt's behavior of showing sources as suggested in issue #630

A new rendered template field is included too: freshness which is the sources.json generated by dbt when running dbt source freshness

This adds a new node type (source), which changes some tests behavior.
This PR also updates the dev dbt project jaffle_shop to include source nodes when enabled.

As seen in the image, source nodes with freshness checks are rendered with a blue color, while the ones rendered as EmptyOperator show a white/light green color

Related Issue(s)

Closes: #630
Closes: #572
Closes: #875

Breaking Change?

This won't be a breaking change since the default behavior will still be ignoring this new feature. That can be changed with the new RenderConfig variable called source_rendering_behavior.

Checklist

• I have made corresponding changes to the documentation (if required)
• I have added tests that prove my fix is effective or that my feature works

[netlify]

✅ Deploy Preview for sunny-pastelito-5ecb04 canceled.

Name	Link
🔨 Latest commit	bacd19d
🔍 Latest deploy log	https://app.netlify.com/sites/sunny-pastelito-5ecb04/deploys/66bb771ef7bf7900085ce719

[arojasb3]

hey @pankajastro !
The errors from the tests are regarding the source node group naming ("source" instead of "run") which can be debated. Since we are checking freshness I'm not 100% sure we should still name those tasks "run".

On the other hand, there's a hashing test error I'm not 100% sure how to deal with 🤔 .

Since this PR could affect some users with the custom rendering, I was thinking we could enable this with a flag?

Please let me know any thoughts in this PR.

[pankajastro]

On the other hand, there's a hashing test error I'm not 100% sure how to deal with 🤔 .

The test test_save_dbt_ls_cache checks the hash of the project dir since you are adding a new file sources.yml in the project the hash has changed maybe you can update the hash value in test

astronomer-cosmos/tests/dbt/test_graph.py

Line 1404 in e61f3a3

def test_save_dbt_ls_cache(mock_variable_set, mock_datetime, tmp_dbt_project_dir):

[pankajastro]

The errors from the tests are regarding the source node group naming ("source" instead of "run") which can be debated. Since we are checking freshness I'm not 100% sure we should still name those tasks "run".

If we decide to keep the source suffix then will have to update the test based on DbtResourceType

astronomer-cosmos/tests/airflow/test_graph.py

Line 345 in e61f3a3

def test_create_task_metadata_model(

@tatiana /@pankajkoti any suggestions here?

[tatiana]

Thanks a lot for creating a new PR on this, @arojasb3 , 5he PR is looking great and I'm super excited we'll be merging this and releasing this in 1.6.

Since this PR could affect some users with the custom rendering, I was thinking we could enable this with a flag?

I agree we should have a feature flag, and perhaps we can enable it by default - and users who want to opt-out can do, what do you think? We have a few other similar feature flags in settings.py.

Since we are checking freshness I'm not 100% sure we should still name those tasks "run".

So far we were using run only for models, it feels worth respecting the naming we had for other node types - like you implemented:

            task_id = f"{node.name}_{node.resource_type.value}"

astronomer-cosmos/cosmos/airflow/graph.py

Line 155 in e61f3a3

task_id = f"{node.name}_{node.resource_type.value}"

[tatiana]

The errors from the tests are regarding the source node group naming ("source" instead of "run") which can be debated. Since we are checking freshness I'm not 100% sure we should still name those tasks "run".

If we decide to keep the source suffix then will have to update the test based on DbtResourceType

astronomer-cosmos/tests/airflow/test_graph.py

Line 345 in e61f3a3

def test_create_task_metadata_model(

@tatiana /@pankajkoti any suggestions here?

As mentioned in #1107 (comment), I believe we should be consistent with the rest of Cosmos naming, and have source nodes having task IDs using:

 task_id = f"{node.name}_{node.resource_type.value}"

[pankajastro]

@arojasb3, it looks like we're almost ready to merge. Could you please resolve the conflict and check for failing CI tests? A quick look suggests that we need to adjust a few parameters in the tests.

[tatiana]

@arojasb3 @pankajastro You can have done an outstanding work developing this feature, adjusting to all change requests, adapting the interfaces, making it backwards compatible, making compromises depending on the dbt version, documenting and testing.

This is an amazing piece of work and I can't wait to hear the community feedback once we release it in Cosmos 1.6.

As follow up tickets:

• We'll be improving the test coverage: [Test] Add integration test for source node rendering #1155
• In Cosmos 2.x, we'll change the default SourceRenderingBehavior so more people can benefit from this

I left a minor comment that can be addressed as part of the test coverage improvements, that are not a requirement for the release of this feature.

[tatiana]

Should we keep only this and remove the lines 182-183, that contain the same?

            if use_task_group is True:
                task_id = node.resource_type.value

[pankajastro]

Thanks, @arojasb3, for the contribution; we appreciate it! 🚀

[fabiomx]

Hi!

Sorry for commenting here and now that it's already merged, but it's very small detail.

When implementing this new feature (thanks a lot for this, @arojasb3!), it felt natural to try importing SourceRenderingBehavior directly from cosmos, just like LoadMode or TestBehavior. However, I noticed that SourceRenderingBehavior isn't included in the __init__ file and has to be imported from cosmos.constants.

I think it would be good if SourceRenderingBehavior could be imported in the same way as the others. What do you think about adding it?

P.S. The same might apply to the InvocationMode constant.

[pankajastro]

Hi @fabiomx, Thanks for your feedback! Feel free to submit a PR anytime—I’d be happy to review and merge it.