Use returncode instead of stderr to determine dbt graph loading errors

[cliff-lau-cloverhealth]

Description

Currently, DbtGraph.load_via_dbt_ls raises CosmosLoadDbtException whenever stderr is not empty.

This behavior does not seem consistent, as non-blocking warnings can still make cosmos refuse to continue.

A specific case that I encountered is an OpenBLAS warning that is produced by podman and composer-local-dev on MacOS:

OpenBLAS WARNING - could not determine the L2 cache size on this system, assuming 256k

This warning does not affect how dbt works, but cosmos still throws out an exception.

Related Issue(s)

N/A

Breaking Change?

No

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 amazing-pothos-a3bca0 processing.

Name	Link
🔨 Latest commit	d8dd05a
🔍 Latest deploy log	https://app.netlify.com/sites/amazing-pothos-a3bca0/deploys/6512c76138b663000897b367

[tatiana]

Hi @cliff-lau-cloverhealth , this is looking great; thanks for your contribution!

Please, could you add tests covering this behaviour? It also seems that at least one of the integration tests is failing due to the change: https://github.com/astronomer/astronomer-cosmos/actions/runs/6230291932/job/16926077144?pr=547

We'll aim to release this in 1.1.2.

[cliff-lau-cloverhealth]

Hi @tatiana, just pushed some fixes for the integration tests.

I have also added two dedicated cases to test for:

1. Non-zero returncode: should raise regardless of stderr and stdout
2. Zero returncode with non-empty stderr: should not raise

However I have difficulties setting up the environment for integration testing locally.

It would be nice if you can try to run them through CI.

[tatiana]

Thanks a lot, @cliff-lau-cloverhealth !

I just authorised to run the integration test from this branch:

tests/dbt/test_graph.py:324: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <MagicMock name='Popen.communicate' id='139833912608848'>

    def assert_called_once(self):
        """assert that the mock was called only once.
        """
        if not self.call_count == 1:
            msg = ("Expected '%s' to have been called once. Called %s times.%s"
                   % (self._mock_name or 'mock',
                      self.call_count,
                      self._calls_repr()))
>           raise AssertionError(msg)
E           AssertionError: Expected 'communicate' to have been called once. Called 0 times.

https://github.com/astronomer/astronomer-cosmos/actions/runs/6244170191/job/17063251829?pr=547

Which issues did you have running the integration tests locally? Please reach out in the Airflow stack channel, and I can support you getting it to work locally.

[joppedh]

I am actually running into this issue now. So looking forward to get it merged 💪

[cliff-lau-cloverhealth]

@tatiana, I just fixed my local integration test setup and got those pass locally.

Could you please authorize the CI to run the integration tests again? Thank you for the back and forth.

---

The problem with the setup was that the default profiles.yml and schema.yml for jaffle_shop_python uses a Databrick connection, which expects the DATABRICKS_* environment variables that a general person might not have. I resolved by swapping it into a PostgreSQL-based profile.

It does not seem to make much sense to default a propietary service in an open source project and impose unnecessary barrier for new contributors - is there a particular reason to do so? On the contrary, the dbt project jaffle_shop expects a PostgreSQL connection instead.

Moreover, it is not mentioned in the contributing guide at the time I write this comment.

[tatiana]

Hi @cliff-lau-cloverhealth , I'm sorry you ran into those issues. Thank you for all the constructive feedback!

@tatiana, I just fixed my local integration test setup and got those pass locally.

🎉 That's great, thank you!

Could you please authorize the CI to run the integration tests again? Thank you for the back and forth.

👍

The problem with the setup was that the default profiles.yml and schema.yml for jaffle_shop_python uses a Databrick connection, which expects the DATABRICKS_* environment variables that a general person might not have. I resolved by swapping it into a PostgreSQL-based profile.

It does not seem to make much sense to default a propietary service in an open source project and impose unnecessary barrier for new contributors - is there a particular reason to do so? On the contrary, the dbt project jaffle_shop expects a PostgreSQL connection instead.

Moreover, it is not mentioned in the contributing guide at the time I write this comment.

I fully agree we need to improve the documentation. I logged a ticket so we don't forget this #555.

As I documented in this ticket, unfortunately, dbt Python models currently (26 September 2023) only run on specific proprietary platforms:

In their initial launch, Python models are supported on three popular data platforms: Snowflake, Databricks, and BigQuery/GCP (via Dataproc). Both Databricks and GCP's Dataproc use PySpark as the processing framework. Snowflake uses its framework, Snowpark, which has many similarities to PySpark.
https://docs.getdbt.com/docs/build/python-models#specific-data-platforms

Since we introduced this feature, we wanted to have an (optional to run) integration test to cover them. If contributors run:

Unit tests:

hatch run tests.py3.8-2.4:test-cov

Integration tests:

hatch run tests.py3.8-2.4:test-integration

They do not depend on Databricks. Databricks tests should run only as part of:

hatch run tests.py3.8-2.4:test-integration-expensive

[tatiana]

Thanks a lot for working on this, @cliff-lau-cloverhealth ! Thanks for adding tests, and I'm sorry about the issues running integration tests locally.

Overall, it feels a bit fragile how dbt / dbt plugins currently deal with stdout and stderr. Stderr is used when there are no errors, and stdout is used when there are actual errors without a non-zero code. There have been discussions within the dbt community about this: dbt-labs/dbt-core#5142. Ideally, we'd also improve this in the source.

Your contribution makes Cosmos more reliable and error-proof by using the error codes. It is unlikely we'd have something in the stderr without the adequate error code.

I'll merge this change once the tests pass.

[codecov]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (d70e6d7) 92.41% compared to head (d8dd05a) 92.42%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #547   +/-   ##
=======================================
  Coverage   92.41%   92.42%           
=======================================
  Files          49       49           
  Lines        1952     1954    +2     
=======================================
+ Hits         1804     1806    +2     
  Misses        148      148           

Files	Coverage Δ
cosmos/dbt/graph.py	98.78% <75.00%> (+0.01%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[joppedh]

Thanks for the work on this issue. I am somehow still running into this error

[2023-10-09 05:58:34,124] {dagbag.py:341} ERROR - (astronomer-cosmos) - Failed to import: /opt/airflow/dags/vendor/transform/jaffle_shop/dag.py
Traceback (most recent call last):
  File "/home/airflow/.local/lib/python3.10/site-packages/airflow/models/dagbag.py", line 337, in parse
    loader.exec_module(new_module)
  File "<frozen importlib._bootstrap_external>", line 883, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "/opt/airflow/dags/vendor/transform/jaffle_shop/dag.py", line 32, in <module>
    my_cosmos_dag = DbtDag(
  File "/home/airflow/.local/lib/python3.10/site-packages/cosmos/airflow/dag.py", line 25, in __init__
    DbtToAirflowConverter.__init__(self, *args, **specific_kwargs(**kwargs))
  File "/home/airflow/.local/lib/python3.10/site-packages/cosmos/converter.py", line 148, in __init__
    dbt_graph.load(method=load_mode, execution_mode=execution_mode)
  File "/home/airflow/.local/lib/python3.10/site-packages/cosmos/dbt/graph.py", line 120, in load
    self.load_via_dbt_ls()
  File "/home/airflow/.local/lib/python3.10/site-packages/cosmos/dbt/graph.py", line 243, in load_via_dbt_ls
    raise CosmosLoadDbtException(f"Unable to run dbt ls command due to the error:\n{details}")
cosmos.dbt.graph.CosmosLoadDbtException: Unable to run dbt ls command due to the error:
OpenBLAS WARNING - could not determine the L2 cache size on this system, assuming 256k

Traceback (most recent call last):
  File "/home/airflow/.local/bin/airflow", line 8, in <module>
    sys.exit(main())
  File "/home/airflow/.local/lib/python3.10/site-packages/airflow/__main__.py", line 48, in main
    args.func(args)
  File "/home/airflow/.local/lib/python3.10/site-packages/airflow/cli/cli_parser.py", line 52, in command
    return func(*args, **kwargs)
  File "/home/airflow/.local/lib/python3.10/site-packages/airflow/utils/session.py", line 75, in wrapper
    return func(*args, session=session, **kwargs)
  File "/home/airflow/.local/lib/python3.10/site-packages/airflow/utils/cli.py", line 108, in wrapper
    return f(*args, **kwargs)
  File "/home/airflow/.local/lib/python3.10/site-packages/airflow/cli/commands/dag_command.py", line 449, in dag_test
    dag = dag or get_dag(subdir=args.subdir, dag_id=args.dag_id)
  File "/home/airflow/.local/lib/python3.10/site-packages/airflow/utils/cli.py", line 231, in get_dag
    raise AirflowException(
airflow.exceptions.AirflowException: Dag 'curated_vendor__jaffle_shop_v1' could not be found; either it does not exist or it failed to parse.

airflow@webserver:/opt/airflow/dags/vendor$ pip list | grep astronomer
astronomer-cosmos                        1.1.2

I am using 1.1.2