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.

Sign up for GitHub

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

Jump to bottom

[dagster-dbt] Refactor UnitTestDefinition instantiation in utils.py to use from_dict method #27052

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[dagster-dbt] Refactor UnitTestDefinition instantiation in utils.py to use from_dict method #27052

wants to merge 2 commits into from
+6 −2

Conversation

kang8
Copy link

@kang8 kang8 commented Jan 13, 2025 •
edited
Loading

Fixed: #27007

Summary & Motivation

Today I upgraded dbt to 1.9.1 but encountered an error while running dagster.
In the end, I found that UnitTestDefinition was not properly parsed.

In dbt, this is also how they create UnitTestDefinition.

Here are the details:

Updated the instantiation of UnitTestDefinition in the __getattr__
method to utilize the from_dict method instead of the constructor. This
change enhances the clarity and consistency of the code when handling
unit test nodes from the manifest JSON.

How I Tested These Changes

Verified test_dbt_with_unit_tests() passes with both fqn:* and tag:test selectors.

dagster/python_modules/libraries/dagster-dbt/dagster_dbt_tests/core/test_asset_decorator.py

Lines 1145 to 1162 in 652af9a

@pytest.mark.skipif(
version.parse(dbt_version) < version.parse("1.8.0"),
reason="dbt unit test support is only available in `dbt-core>=1.8.0`",
)
@pytest.mark.parametrize("select", ["fqn:*", "tag:test"])
def test_dbt_with_unit_tests(test_dbt_unit_tests_manifest: dict[str, Any], select: str) -> None:
@dbt_assets(
manifest=test_dbt_unit_tests_manifest,
select=select,
)
def my_dbt_assets(context: AssetExecutionContext, dbt: DbtCliResource):
yield from dbt.cli(["build"], context=context).stream()
result = materialize(
[my_dbt_assets],
resources={"dbt": DbtCliResource(project_dir=os.fspath(test_dbt_unit_tests_path))},
)
assert result.success

@kang8 kang8 force-pushed the fix/dbt-unit-test-definition-from-dict branch from 652af9a to 2569dbe Compare January 13, 2025 12:00
@maximearmstrong
Copy link
Contributor

Hey kang8 - Thanks for your contribution!

I'm in favor of this change. What was the error message you received before? Would it possible to add a new unit test to make sure the use case is covered?

kang8 added 2 commits January 14, 2025 10:52
@kang8
[dagster-dbt] Refactor UnitTestDefinition instantiation in utils.py t…
1559c25 
…o use from_dict method

Verified `test_dbt_with_unit_tests()` passes with both "fqn:*" and "tag:test" selectors.

Updated the instantiation of UnitTestDefinition in the __getattr__
method to utilize the from_dict method instead of the constructor. This
change enhances the clarity and consistency of the code when handling
unit test nodes from the manifest JSON.

Fixed: dagster-io#27007
@kang8
[dagster-dbt] Let dbt19 also be added to the test
9452c84
@kang8 kang8 force-pushed the fix/dbt-unit-test-definition-from-dict branch from 2569dbe to 9452c84 Compare January 14, 2025 03:02
@kang8
Copy link
Author

kang8 commented Jan 14, 2025 •
edited
Loading

Just like issue #27007, when I upgrade dbt to version 1.9.1 and run dagster dev, an error occurs:

Click to display error message. 
The above exception was caused by the following exception:
AttributeError: 'dict' object has no attribute 'enabled'

Stack Trace:
  File "/Users/kang/self_dagster/.venv/lib/python3.11/site-packages/dagster/_core/errors.py", line 287, in user_code_error_boundary
    yield
  File "/Users/kang/self_dagster/.venv/lib/python3.11/site-packages/dagster/_grpc/server.py", line 258, in __init__
    loadable_targets = get_loadable_targets(
                       ^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kang/self_dagster/.venv/lib/python3.11/site-packages/dagster/_grpc/utils.py", line 50, in get_loadable_targets
    else loadable_targets_from_python_module(module_name, working_directory)
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kang/self_dagster/.venv/lib/python3.11/site-packages/dagster/_core/workspace/autodiscovery.py", line 32, in loadable_targets_from_python_module
    module = load_python_module(
             ^^^^^^^^^^^^^^^^^^^
  File "/Users/kang/self_dagster/.venv/lib/python3.11/site-packages/dagster/_core/code_pointer.py", line 134, in load_python_module
    return importlib.import_module(module_name)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kang/.local/share/uv/python/cpython-3.11.11-macos-aarch64-none/lib/python3.11/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1204, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1176, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1147, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 690, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 940, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "/Users/kang/self_dagster/dagster/__init__.py", line 4, in <module>
    from .assets import (
  File "/Users/kang/self_dagster/dagster/assets/__init__.py", line 2, in <module>
    from .customer_io import customer_io_assets
  File "/Users/kang/self_dagster/dagster/assets/customer_io.py", line 5, in <module>
    from ..assets.dbt import rightcapital_dbt_assets
  File "/Users/kang/self_dagster/dagster/assets/dbt.py", line 47, in <module>
    @dbt_assets(
     ^^^^^^^^^^^
  File "/Users/kang/self_dagster/.venv/lib/python3.11/site-packages/dagster/_core/decorator_utils.py", line 223, in wrapped_with_context_manager_fn
    return fn(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^
  File "/Users/kang/self_dagster/.venv/lib/python3.11/site-packages/dagster_dbt/asset_decorator.py", line 305, in dbt_assets
    specs, check_specs = build_dbt_specs(
                         ^^^^^^^^^^^^^^^^
  File "/Users/kang/self_dagster/.venv/lib/python3.11/site-packages/dagster_dbt/asset_utils.py", line 806, in build_dbt_specs
    selected_unique_ids = select_unique_ids_from_manifest(
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kang/self_dagster/.venv/lib/python3.11/site-packages/dagster_dbt/utils.py", line 115, in select_unique_ids_from_manifest
    selector = graph_selector.NodeSelector(graph, manifest)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kang/self_dagster/.venv/lib/python3.11/site-packages/dbt/graph/selector.py", line 52, in __init__
    graph_members = {
                    ^
  File "/Users/kang/self_dagster/.venv/lib/python3.11/site-packages/dbt/graph/selector.py", line 53, in <setcomp>
    unique_id for unique_id in self.full_graph.nodes() if self._is_graph_member(unique_id)
                                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/kang/self_dagster/.venv/lib/python3.11/site-packages/dbt/graph/selector.py", line 177, in _is_graph_member
    return unit_test.config.enabled
           ^^^^^^^^^^^^^^^^^^^^^^^^

  warnings.warn(f"Error loading repository location {location_name}:{error.to_string()}")

Then I spent some time testing and found that there are currently no tests covering dbt 1.9. I have added tests for dbt 1.9 in 9452c84.

PS: This is my first time learning about tox and buildkite, I'm not sure if I added the dbt 1.9 test correctly, if there are any issues, please feel free to point them out.

Would it possible to add a new unit test to make sure the use case is covered?

Adding support for dbt 1.9 in the test is enough, because the existing tests have already covered this situation, it's just that the dbt 1.9 version was not tested before.

@garethbrickman garethbrickman requested review from maximearmstrong and removed request for maximearmstrong January 21, 2025 20:08
@garethbrickman garethbrickman mentioned this pull request Jan 21, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maximearmstrong maximearmstrong Awaiting requested review from maximearmstrong

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dagster 1.9.7 fails to initialize if a dbt model has unit tests
2 participants
@kang8 @maximearmstrong