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

Refactor code to properly handle reference deprecations #10852

Merged
merged 8 commits into from
Oct 15, 2024

Conversation

gshank
Copy link
Contributor

@gshank gshank commented Oct 15, 2024

Resolves #10833

Problem

Due to some code changes we stopped reporting deprecation/upcoming deprecations for models referencing a model with a deprecation_date.

Solution

Check models that refer to a model with a deprecation date.

Checklist

  • I have read the contributing guide and understand what's expected of me.
  • I have run this code in development, and it appears to resolve the stated issue.
  • This PR includes tests, or tests are not required or relevant for this PR.
  • This PR has no interface changes (e.g., macros, CLI, logs, JSON artifacts, config files, adapter interface, etc.) or this PR has already received feedback and approval from Product or DX.
  • This PR includes type annotations for new and modified functions.

@gshank gshank requested a review from a team as a code owner October 15, 2024 16:16
@cla-bot cla-bot bot added the cla:yes label Oct 15, 2024
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

Copy link

codecov bot commented Oct 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.15%. Comparing base (ef9abe6) to head (ccea067).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10852      +/-   ##
==========================================
- Coverage   89.18%   89.15%   -0.03%     
==========================================
  Files         183      183              
  Lines       23430    23430              
==========================================
- Hits        20896    20889       -7     
- Misses       2534     2541       +7     
Flag Coverage Δ
integration 86.43% <100.00%> (-0.06%) ⬇️
unit 62.12% <0.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
Unit Tests 62.12% <0.00%> (+0.01%) ⬆️
Integration Tests 86.43% <100.00%> (-0.06%) ⬇️

Copy link
Contributor

@ChenyuLInx ChenyuLInx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this.
Given this is a regression, let's add a test to make sure this doesn't happen again.

@gshank
Copy link
Contributor Author

gshank commented Oct 15, 2024

There was a test. The problem was that the test wasn't being run because it was named "model_deprecations.py" instead of "test_model_deprecations.py".

@gshank gshank requested a review from ChenyuLInx October 15, 2024 18:16
Copy link
Contributor

@peterallenwebb peterallenwebb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good, but it doesn't look like we have a test that would fail if this behavior were to regress again? If I'm right about that, can you please add one?

@gshank gshank merged commit ffa75ca into main Oct 15, 2024
61 of 62 checks passed
@gshank gshank deleted the warning_for_deprecated_model_references branch October 15, 2024 20:44
@danlsn
Copy link

danlsn commented Oct 23, 2024

Hi there, this issue seems to have caused failures during parsing in our dbt cloud project using the "versionless" version.

06:43:39 Encountered an error:
'exposure.dap_dbt_da.research_plus_hdr_student_activity_report'
06:43:39 Traceback (most recent call last):
  File "/venv/dbt-149263e0c5a7b047bf5cd7b44643df173f2f8287/lib/python3.11/site-packages/dbt/cli/requires.py", line 221, in wrapper
    result, success = func(*args, **kwargs)
                      ^^^^^^^^^^^^^^^^^^^^^
  File "/venv/dbt-149263e0c5a7b047bf5cd7b44643df173f2f8287/lib/python3.11/site-packages/dbt/cli/requires.py", line 131, in wrapper
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/venv/dbt-149263e0c5a7b047bf5cd7b44643df173f2f8287/lib/python3.11/site-packages/dbt/cli/requires.py", line 329, in wrapper
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/venv/dbt-149263e0c5a7b047bf5cd7b44643df173f2f8287/lib/python3.11/site-packages/dbt/cli/requires.py", line 356, in wrapper
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/venv/dbt-149263e0c5a7b047bf5cd7b44643df173f2f8287/lib/python3.11/site-packages/dbt/cli/requires.py", line 404, in wrapper
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/venv/dbt-149263e0c5a7b047bf5cd7b44643df173f2f8287/lib/python3.11/site-packages/dbt/cli/requires.py", line 420, in wrapper
    setup_manifest(
  File "/venv/dbt-149263e0c5a7b047bf5cd7b44643df173f2f8287/lib/python3.11/site-packages/dbt/cli/requires.py", line 469, in setup_manifest
    ctx.obj["manifest"] = parse_manifest(
                          ^^^^^^^^^^^^^^^
  File "/venv/dbt-149263e0c5a7b047bf5cd7b44643df173f2f8287/lib/python3.11/site-packages/dbt/parser/manifest.py", line 2055, in parse_manifest
    manifest = ManifestLoader.get_full_manifest(
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/venv/dbt-149263e0c5a7b047bf5cd7b44643df173f2f8287/lib/python3.11/site-packages/dbt/parser/manifest.py", line 318, in get_full_manifest
    manifest = loader.load()
               ^^^^^^^^^^^^^
  File "/venv/dbt-149263e0c5a7b047bf5cd7b44643df173f2f8287/lib/python3.11/site-packages/dbt/parser/manifest.py", line 521, in load
    self.check_for_model_deprecations()
  File "/venv/dbt-149263e0c5a7b047bf5cd7b44643df173f2f8287/lib/python3.11/site-packages/dbt/parser/manifest.py", line 606, in check_for_model_deprecations
    child_node = self.manifest.nodes[child_unique_id]
                 ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^
KeyError: 'exposure.dap_dbt_da.research_plus_hdr_student_activity_report'

This error seems to occur when our exposures depend on versioned nodes.

It seems like the fqn of the of the exposure is being used as the child_unique_id to check for deprecated models. Not entirely sure.

Our platform team is reaching out to our contact at dbt-labs but I figured it would be worth raising here as this seems to have caused another regression and is currently blocking development for our production work.

Happy to provide more information if required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants