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

Added quality-of-life feature to make model_type lookup case-insensitive in the model_audit macro #61

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RyanBalshaw
Copy link

This is a small quality of life change that makes the lookup for the model_type key in the ml_config dictionary case insensitive.

In the dbt_ml.model_audit macro, I observed that the model_type is inferred from the ml_config dictionary and is used to write some template information to the model audit table in BigQuery. However, the key 'model_type' is explicitly required to be lower case.

This merge request primarily stems from an issue I encountered (see the discussion in #56) where if I used "MODEL_TYPE": "ARIMA_PLUS" in the ml_config dictionary I hit issues when using the post-hook:

models:
  <project>:
    ml:
      enabled: true
      schema: ml
      materialized: model
      post-hook: "{{ dbt_ml.model_audit() }}"

The case requirement is not immediately clear if one compares the dbt_ml syntax in the README to the BigQuery ML syntax guide.

There are three paths to remedy this:

  • Make it clear in the README.md file that model_type is supposed to be lower case.
  • Use the changes to the macro contained within this merge request.
  • Accept that this is a highly unlikely issue for users to face and ignore this MR.

I do not have a preference. Please choose the path that you think is most sensible.

Please let me know if you require me to first create an issue and then link this merge request to an issue, I could not find any contribution guidelines.

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

Successfully merging this pull request may close these issues.

1 participant