Correctly handle an empty dbt_project.yml in `dbt debug` #2120

nchammas · 2020-02-11T18:27:17Z

It's confusing to access the dbt project from within _load_profile() since the project has not been validated yet. If the project is invalid, the code validating the profile fails.

I've tried to handle this case gracefully using the existing patterns.

Old behavior:

$ dbt debug
Running with dbt=0.15.2
dbt version: 0.15.2
python version: 3.7.6
python path: .../venv/bin/python
os info: Darwin-18.7.0-x86_64-i386-64bit
Using profiles.yml file at .../.dbt/profiles.yml

Encountered an error:
'NoneType' object does not support item assignment

New behavior:

$ dbt debug
Running with dbt=0.16.0-b1
dbt version: 0.16.0-b1
python version: 3.7.6
python path: .../dbt/env/bin/python
os info: Darwin-18.7.0-x86_64-i386-64bit
Using profiles.yml file at .../.dbt/profiles.yml
Using dbt_project.yml file at .../dbt/dbt_project.yml

Configuration:
  profiles.yml file [OK found and valid]
  dbt_project.yml file [ERROR invalid]

Project loading failed for the following reason:
Runtime Error
  dbt_project.yml does not parse to a dictionary

Required dependencies:
 - git [OK found]

Could not load dbt_project.yml

Fixes #2116.

cla-bot · 2020-02-11T18:27:19Z

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin.

CLA has not been signed by users: @nchammas

nchammas · 2020-02-11T18:32:41Z

core/dbt/config/project.py

@@ -189,6 +189,12 @@ def _raw_project_from(project_root: str) -> Dict[str, Any]:
        )

    project_dict = _load_yaml(project_yaml_filepath)


Ideally, we should just validate the complete project schema here, perhaps using something like StrictYAML.

This is pretty cool! I'll have to look into it. Sadly it looks like they don't support anchors, but maybe we'd live without that. We do validate the parsed project using hologram, but I would love a smaller, more close-to-the-input schema definition for your yaml files.

What are anchors?

foo: &my_anchor a: 1 b: [2, 3] c: false bar: *my_anchor

Is equivalent to:

foo: a: 1 b: [2, 3] c: false bar: a: 1 b: [2, 3] c: false

There's also a field-override syntax, which is very convenient.

Ah yes, StrictYAML removes a lot of YAML features like that.

I'm not familiar with Hologram, but I have heard of Cerberus as an option for flexible data validation.

core/dbt/task/debug.py

drewbanin · 2020-02-11T18:42:16Z

Thanks for opening this PR @nchammas! @cla-bot check

Adding @beckjake to review :)

cla-bot · 2020-02-11T18:42:19Z

The cla-bot has been summoned, and re-checked this pull request!

core/dbt/task/debug.py

beckjake

Hey @nchammas, this looks great! Thanks for your contribution. Do you think you could add a test? You should be able to copy+paste and change one of the tests in test/integration/049_debug_tests/test_debug.py to generate an empty profile (I'd probably start with the test_postgres_invalid_project_outside_current_dir test).

beckjake · 2020-02-12T14:51:17Z

core/dbt/config/project.py

@@ -189,6 +189,12 @@ def _raw_project_from(project_root: str) -> Dict[str, Any]:
        )

    project_dict = _load_yaml(project_yaml_filepath)


This is pretty cool! I'll have to look into it. Sadly it looks like they don't support anchors, but maybe we'd live without that. We do validate the parsed project using hologram, but I would love a smaller, more close-to-the-input schema definition for your yaml files.

core/dbt/task/debug.py

nchammas · 2020-02-12T17:52:13Z

@beckjake - I'm trying to run that test locally, but it seems the way we've setup DBTIntegrationTest.database_host() makes it so you can't run tests locally:

$ pytest -k test_postgres_invalid_project_outside_current_dir
...
>           raise dbt.exceptions.FailedToConnectException(str(e))
E           dbt.exceptions.FailedToConnectException: Database Error
E             could not translate host name "database" to address: nodename nor servname provided, or not known

.../dbt/plugins/postgres/dbt/adapters/postgres/connections.py:111: FailedToConnectException

Here's the culprit: https://github.com/fishtown-analytics/dbt/blob/4e58589afdbb5e74701753e239536e318c5d8db7/test/integration/base.py#L128-L131

Why does that method return database? And what's the proper way for me to override that? The test.env.sample does not show anything for Postgres, and trying stuff like POSTGRES_TEST_HOST or POSTGRESQL_TEST_HOST in my test.env doesn't seem to change how dbt tries to connect to the database.

beckjake · 2020-02-12T19:08:05Z

@nchammas I recommend using docker-compose! The test assume you're using the database in our docker-compose.yml and that you've run the setup script. From the dbt repository checkout:

docker-compose up -d postgres
docker-compose run --rm test /bin/bash ./test/setup_db.sh
docker-compose run --rm test tox -e explicit-py36 -- -s -m profile_postgres ./test/integration/049_dbt_debug_test/test_debug.py

I think that will get your tests working properly.

nchammas · 2020-02-12T21:42:16Z

OK, I'll try that. The contributing guide isn't clear that this is a requirement for running the tests, but then again it doesn't suggest calling pytest directly either. 😄

nchammas · 2020-02-12T21:47:55Z

I believe you meant docker-compose up -d database, no?

And on the third step you provided I'm getting the same error I reported in #2128.

beckjake · 2020-02-12T22:21:33Z

Oops, yes, you're correct - postgres is the image name, database is the service name!

As I commented on #2128, something is broken with docker image building (I obviously don't do it often!). The circleci tests use fishtownjacob/test-container:3 for their image, instead of building the local docker image - you can always substitute that in to the docker-compose file to get running locally.

nchammas · 2020-02-12T22:56:01Z

I'm getting the same error with fishtownjacob/test-container:3. Are you able to reproduce the issue locally?

beckjake · 2020-02-13T14:27:18Z

Yeah, I am. I didn't really want to spend my day on this but I guess I'm going to have to figure out what's wrong since I can't run tests anymore.

nchammas · 2020-02-13T20:49:05Z

Tests pass locally with:

docker-compose run --rm test tox -e explicit-py36 -- -s -m profile_postgres \
    ./test/integration/049_dbt_debug_test/test_debug.py::TestDebugInvalidProject

By the way, that set of 4 tests alone takes 12 seconds to run. Is that normal?

nchammas · 2020-02-14T16:04:15Z

Not sure why some of the checks aren't running, but the ones that did run, passed.

beckjake

Not sure why some of the checks aren't running, but the ones that did run, passed.

The ones that aren't running require manual intervention to run. I'll kick them off, thanks for your contribution!

beckjake · 2020-02-14T16:25:35Z

@nchammas before I merge/kick off tests, could you rebase this against dev/barbara-gittings, and squash it into one commit? Those tests shouldn't fail if these passed.

nchammas · 2020-02-14T19:02:01Z

I did recently merge dev/barbara-gittings into this branch as part of
5595267, and GitHub lets you squash and merge PRs, which is what I normally do to merge PRs.

Is that not good enough? If not, I can manually rebase and squash as you request, but it seems like something we shouldn't need to do.

beckjake · 2020-02-18T15:21:54Z

@nchammas sorry, I haven't been attentive to github! We have squash and merge disabled so I actually can't do it at merge-time.

nchammas · 2020-02-18T16:46:30Z

OK, done.

beckjake · 2020-02-18T17:01:28Z

Thanks! I've kicked off the tests. Once they pass, I'll merge this.

nchammas commented Feb 11, 2020

View reviewed changes

cla-bot bot added the cla:yes label Feb 11, 2020

nchammas commented Feb 11, 2020

View reviewed changes

core/dbt/task/debug.py Show resolved Hide resolved

beckjake suggested changes Feb 12, 2020

View reviewed changes

nchammas mentioned this pull request Feb 12, 2020

make test-unit failing with ImportError: cannot import name 'ensure_text' #2128

Closed

1 task

nchammas force-pushed the dbt-debug-empty-project branch 2 times, most recently from 440b9ba to cf90232 Compare February 13, 2020 20:24

nchammas requested a review from beckjake February 14, 2020 16:04

beckjake approved these changes Feb 14, 2020

View reviewed changes

handle empty dbt_project

5dc9393

nchammas force-pushed the dbt-debug-empty-project branch from dcd50a5 to 5dc9393 Compare February 18, 2020 16:32

beckjake merged commit ea4948f into dbt-labs:dev/barbara-gittings Feb 18, 2020

nchammas deleted the dbt-debug-empty-project branch February 18, 2020 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly handle an empty dbt_project.yml in `dbt debug` #2120

Correctly handle an empty dbt_project.yml in `dbt debug` #2120

nchammas commented Feb 11, 2020 •

edited

Loading

cla-bot bot commented Feb 11, 2020

nchammas Feb 11, 2020 •

edited

Loading

beckjake Feb 12, 2020

nchammas Feb 12, 2020

beckjake Feb 14, 2020

nchammas Feb 14, 2020

drewbanin commented Feb 11, 2020

cla-bot bot commented Feb 11, 2020

beckjake left a comment

beckjake Feb 12, 2020

nchammas commented Feb 12, 2020

beckjake commented Feb 12, 2020

nchammas commented Feb 12, 2020

nchammas commented Feb 12, 2020

beckjake commented Feb 12, 2020

nchammas commented Feb 12, 2020

beckjake commented Feb 13, 2020

nchammas commented Feb 13, 2020 •

edited

Loading

nchammas commented Feb 14, 2020

beckjake left a comment

beckjake commented Feb 14, 2020

nchammas commented Feb 14, 2020

beckjake commented Feb 18, 2020

nchammas commented Feb 18, 2020

beckjake commented Feb 18, 2020

		@@ -189,6 +189,12 @@ def _raw_project_from(project_root: str) -> Dict[str, Any]:
		)

		project_dict = _load_yaml(project_yaml_filepath)

Correctly handle an empty dbt_project.yml in dbt debug #2120

Correctly handle an empty dbt_project.yml in dbt debug #2120

Conversation

nchammas commented Feb 11, 2020 • edited Loading

cla-bot bot commented Feb 11, 2020

nchammas Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

beckjake Feb 12, 2020

Choose a reason for hiding this comment

nchammas Feb 12, 2020

Choose a reason for hiding this comment

beckjake Feb 14, 2020

Choose a reason for hiding this comment

nchammas Feb 14, 2020

Choose a reason for hiding this comment

drewbanin commented Feb 11, 2020

cla-bot bot commented Feb 11, 2020

beckjake left a comment

Choose a reason for hiding this comment

beckjake Feb 12, 2020

Choose a reason for hiding this comment

nchammas commented Feb 12, 2020

beckjake commented Feb 12, 2020

nchammas commented Feb 12, 2020

nchammas commented Feb 12, 2020

beckjake commented Feb 12, 2020

nchammas commented Feb 12, 2020

beckjake commented Feb 13, 2020

nchammas commented Feb 13, 2020 • edited Loading

nchammas commented Feb 14, 2020

beckjake left a comment

Choose a reason for hiding this comment

beckjake commented Feb 14, 2020

nchammas commented Feb 14, 2020

beckjake commented Feb 18, 2020

nchammas commented Feb 18, 2020

beckjake commented Feb 18, 2020

Correctly handle an empty dbt_project.yml in `dbt debug` #2120

Correctly handle an empty dbt_project.yml in `dbt debug` #2120

nchammas commented Feb 11, 2020 •

edited

Loading

nchammas Feb 11, 2020 •

edited

Loading

nchammas commented Feb 13, 2020 •

edited

Loading