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

Add --quiet flag and print Jinja function #4701

Merged
merged 5 commits into from
Feb 10, 2022

Conversation

ehmartens
Copy link
Contributor

@ehmartens ehmartens commented Feb 9, 2022

resolves #3451

Description

This PR addresses the issue linked above following @emmyoop 's suggestions here.

It creates a new global flag called --quiet that suppresses all non-exception output. Additionally it create a Jinja function called print whihc prints messages directly to stdout.

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change

@ehmartens ehmartens requested review from a team as code owners February 9, 2022 16:01
@ehmartens ehmartens requested review from nathaniel-may and stu-k and removed request for a team February 9, 2022 16:01
@cla-bot
Copy link

cla-bot bot commented Feb 9, 2022

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: @ehmartens

Copy link
Contributor

@nathaniel-may nathaniel-may 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 your work here, @ehmartens! This looks like exactly the right implementation. I just left a few minor comments, and I'd like to get @emmyoop's eyes on it too before merging but otherwise this looks real good to me.

Blockers:

  • Just the spelling fix in the help message for the new flag.

@@ -1084,6 +1084,17 @@ def parse_args(args, cls=DBTArgumentParser):
'''
)

p.add_argument(
'-q',
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with this, but I want to highlight the short argument for other reviewers to make sure we want that in addition to the long --quiet.

Copy link
Member

Choose a reason for hiding this comment

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

I like short arguments so I'm happy with this!

core/dbt/main.py Outdated
Comment on lines 1093 to 1094
Supress all non-error logging during dbt execution. Output from
{{ print() }} macro are still displayed.
Copy link
Contributor

Choose a reason for hiding this comment

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

I just wanted to be clear about the stdout part, fixed a spelling mistake, and shifted wording around a bit. If anyone has a better way to phrase this I'm down for that too.

Suggested change
Supress all non-error logging during dbt execution. Output from
{{ print() }} macro are still displayed.
Suppress all non-error logging to stdout. Does not affect
{{ print() }} macro calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

@@ -69,3 +69,7 @@ def test__postgres_select(self):
@use_profile('postgres')
def test__postgres_access_graph(self):
self.run_operation('log_graph')

@use_profile('postgres')
def test__postgres_print(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like a comment on this test describing precisely what we're testing. In this case I think it's just checking that calling the print macro doesn't blow up, which imo is good enough test coverage for me.

Copy link
Member

Choose a reason for hiding this comment

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

Agree that a comment here would be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added!

@@ -115,7 +115,7 @@ def test__flags(self):
os.environ['DBT_DEBUG'] = 'True'
flags.set_from_args(self.args, self.user_config)
self.assertEqual(flags.DEBUG, True)
os.environ['DBT_DEUBG'] = 'False'
os.environ['DBT_DEBUG'] = 'False'
Copy link
Contributor

Choose a reason for hiding this comment

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

oh nice, good catch.

@nathaniel-may nathaniel-may requested a review from emmyoop February 9, 2022 18:38
Copy link
Member

@emmyoop emmyoop left a comment

Choose a reason for hiding this comment

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

Agree with @nathaniel-may, this looks great! Thanks for your work on this and opening up an issue over in codegen.

I cloned this locally to play with it and have one concern.

I have this useless macro that I stuck into a pre_hook for a model for testing purposes.

{% macro do_it() %}

    {{log('********************This is a LOG line!', True)}}
    {{print('********************This is a PRINT line!')}}

{% endmacro %}

It works as expected. dbt -quiet run prints the print() statement and nothing else but the logs all still go to the file. Great!

My concern is that when I don't use the -quiet flag the print statement still comes out.

(env) emily@Emily-Rockman basic-dbt % dbt run
19:12:19  Running with dbt=1.0.1
19:12:19  ********************This is a LOG line!
********************This is a PRINT line!
19:12:19  Found 5 models, 7 tests, 0 snapshots, 0 analyses, 354 macros, 0 operations, 1 seed file, 0 sources, 1 exposure, 0 metrics
19:12:19
19:12:19  Concurrency: 4 threads (target='dev')
19:12:19
19:12:19  1 of 5 START table model dbt_testing.even_id_table.............................. [RUN]
19:12:19  2 of 5 START view model dbt_testing.male_list_view.............................. [RUN]
19:12:19  3 of 5 START table model dbt_testing.my_first_dbt_model......................... [RUN]
19:12:19  ********************This is a LOG line!
********************This is a PRINT line!
19:12:20  2 of 5 OK created view model dbt_testing.male_list_view......................... [CREATE VIEW in 0.31s]
19:12:20  3 of 5 OK created table model dbt_testing.my_first_dbt_model.................... [SELECT 2 in 0.32s]
19:12:20  1 of 5 OK created table model dbt_testing.even_id_table......................... [SELECT 50 in 0.33s]
19:12:20  4 of 5 START view model dbt_testing.my_second_dbt_model......................... [RUN]
19:12:20  5 of 5 START view model dbt_testing.clean_table................................. [RUN]
19:12:20  4 of 5 OK created view model dbt_testing.my_second_dbt_model.................... [CREATE VIEW in 0.07s]
19:12:20  5 of 5 OK created view model dbt_testing.clean_table............................ [CREATE VIEW in 0.07s]
19:12:20
19:12:20  Finished running 3 view models, 2 table models in 0.61s.
19:12:20
19:12:20  Completed successfully
19:12:20
19:12:20  Done. PASS=5 WARN=0 ERROR=0 SKIP=0 TOTAL=5

This completely makes sense for how I said to implement it but I'm wondering if it's really desired behavior here.

@@ -19,6 +20,7 @@
Contributors:
- [@NiallRees](https://github.com/NiallRees) ([#4447](https://github.com/dbt-labs/dbt-core/pull/4447))
- [@alswang18](https://github.com/alswang18) ([#4644](https://github.com/dbt-labs/dbt-core/pull/4644))
- [@emartens](https://github.com/ehmartens) ([#4701](https://github.com/dbt-labs/dbt-core/pull/4701))
Copy link
Member

Choose a reason for hiding this comment

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

🎉

core/dbt/main.py Outdated
Comment on lines 1092 to 1095
help='''
Supress all non-error logging during dbt execution. Output from
{{ print() }} macro are still displayed.
'''
Copy link
Member

Choose a reason for hiding this comment

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

@jtcohen6 can I get your eyes on this help text?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@emmyoop I updated this to the suggestion that @nathaniel-may made here. Apologies if I should have waited!

@jaypeedevlin
Copy link
Contributor

jaypeedevlin commented Feb 9, 2022

@emmyoop Chiming in on the expected behavior her (I paired with Elizabeth on this work this morning) and correct me if I'm wrong: To use the codegen example, if we replaced all the log()s with print()s and suppressed print() unless --quiet was used, users would have to use --quiet to get the output. In my mind at least, that's probably not the behavior we want, but I will (of course) to defer to your collective judgement.

@nathaniel-may
Copy link
Contributor

nathaniel-may commented Feb 9, 2022

@emmyoop thanks for the example output! Although it looks kind of awkward, I think this is the correct behavior for the --quiet flag. If we think there's a use case for turning off print statements, I might suggest another flag like --no-print, or --logs-only so each user can run with the exact behavior they're looking for.

@emmyoop
Copy link
Member

emmyoop commented Feb 9, 2022

@jaypeedevlin I completely agree that the behavior when using the --quiet flag is right.

My concern is macros using print() will always have whatever they're printing come to stdout. This is more of a concern for macros outside codegen or really any macros meant to be run with run-operation. I was just surprised (though I should not have been) when I saw stdout muddied a bit with the print statement. I think the way I tested the print statement is not the intended method so maybe the right solution is just some docs pointing out the behavior.

Copy link
Member

@emmyoop emmyoop left a comment

Choose a reason for hiding this comment

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

@ehmartens and @jaypeedevlin thanks for the quick work on this and the patience as we figured out the best way to solve this specific problem with the best all around solution for dbt-core as a whole!

Copy link
Contributor

@nathaniel-may nathaniel-may left a comment

Choose a reason for hiding this comment

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

Excellent work here. Thanks for contributing!

@emmyoop emmyoop merged commit 41c6177 into dbt-labs:main Feb 10, 2022
@nhammad
Copy link

nhammad commented Mar 22, 2022

Currently, I get an output that looks like this:

dbt run-operation generate_model_yaml --args "{\"model_name\": "bookings"}"  > test.yml  
12:53:32  Running with dbt=1.0.1
12:53:34  version: 2

models:
  - name: bookings
    description: ""
    columns:
      - name: booking_id
        description: ""

      - name: masterclient_id
        description: ""

Can I use the --quite argument to get rid of the timestamps etc? I tried this:

dbt run-operation generate_model_yaml --quiet --args "{\"model_name\": \"$table\"}" > $target_dir/$table.yml

but:

dbt: error: unrecognized arguments: --quiet

@jaypeedevlin
Copy link
Contributor

Hey @nhammad two things to note here:

  1. The new flag has not been released (it's scheduled for 1.1.0 I believe)
  2. The flag by itself won't change the behavior of codegen, however the linked issue in that repo (when addressed) in combination with this will.

mirnawong1 added a commit to dbt-labs/docs.getdbt.com that referenced this pull request Jan 23, 2024
[Preview](https://docs-getdbt-com-git-dbeatty-print-no-print-dbt-labs.vercel.app/reference/global-configs/print-output#suppress-print-messages-in-stdout)

resolves #4776

## What are you changing in this pull request and why?

I checked from v1.7 back to v1.1, and I couldn't get either of these to
work in any version:

Up to 1.5:
```yaml
config:
  no_print: true
```

1.5 and after:
```yaml
config:
  print: false
```

However, the `DBT_NO_PRINT` / `DBT_PRINT` environment variables and
`--no-print` and `--print` CLI flags _do_ work.

So I think the code example for `profiles.yml` was just accidentally
introduced in the following PRs, and we should remove it because it's
not actually an option for any versions.
- #1319
- #3134

For additional context, see:
- [Upgrading to
v1.1](https://docs.getdbt.com/docs/dbt-versions/core-upgrade/upgrading-to-v1.1#advanced-and-experimental-functionality)
- [Upgrading to
v1.5](https://docs.getdbt.com/docs/dbt-versions/core-upgrade/upgrading-to-v1.5#behavior-changes)
- #1102
- dbt-labs/dbt-core#4701
- #3122
- #3332

## Checklist
- [x] Review the [Content style
guide](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/content-style-guide.md)
so my content adheres to these guidelines.
- [x] For [docs
versioning](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/single-sourcing-content.md#about-versioning),
review how to [version a whole
page](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/single-sourcing-content.md#adding-a-new-version)
and [version a block of
content](https://github.com/dbt-labs/docs.getdbt.com/blob/current/contributing/single-sourcing-content.md#versioning-blocks-of-content).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-172] --quiet flag for run-operations
5 participants