Skip to content

Lower various logging statement levels to clean up example printing#30782

Merged
tvalentyn merged 2 commits intoapache:masterfrom
hjtran:updatelogging
Apr 5, 2024
Merged

Lower various logging statement levels to clean up example printing#30782
tvalentyn merged 2 commits intoapache:masterfrom
hjtran:updatelogging

Conversation

@hjtran
Copy link
Copy Markdown
Contributor

@hjtran hjtran commented Mar 28, 2024

The playground examples have tons of info logging that drown out any actual logging that the example might've caused. This is somewhat of a big hit to the playground experience (not just noted by me, but by others that I've been introducing to Beam)

I attempted a fix to change the playground's log level but that didn't seem to take. Vlado Djerek also tried to figure out where the logging level is set but was also unsuccessful.

In lieu of a real fix, I just changed or deleted various logging statements that I suspect are probably not all that useful. I suspect especially that the translation prints which really clog up the examples don't offer much useful information.

Here's an example of what running the example looks like right now:
image

@hjtran
Copy link
Copy Markdown
Contributor Author

hjtran commented Mar 28, 2024

reviewer: @tvalentyn

@hjtran
Copy link
Copy Markdown
Contributor Author

hjtran commented Mar 28, 2024

I left a few statements out because they seemed like they're more important, but I think they do still distract from the examples. If we ever find where the log level is set, I wonder if we should just disable the logging altogether and update any examples that use the logger to just use beam.Map(print) or beam.LogElements instead

@github-actions
Copy link
Copy Markdown
Contributor

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.44%. Comparing base (827e96d) to head (4ed3742).
Report is 135 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #30782       +/-   ##
===========================================
+ Coverage   38.52%   71.44%   +32.91%     
===========================================
  Files         698      710       +12     
  Lines      102371   104812     +2441     
===========================================
+ Hits        39440    74878    +35438     
+ Misses      61301    28304    -32997     
  Partials     1630     1630               
Flag Coverage Δ
python 81.22% <100.00%> (+52.07%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tvalentyn
Copy link
Copy Markdown
Contributor

I attempted a fix to change the playground's #29948 but that didn't seem to take. Vlado Djerek also tried to figure out where the logging level is set but was also unsuccessful.

By crashing playground intepreter, I see that the main python entrypoint is located in some generated files, like:
/opt/playground/backend/executable_files/8ff1f92b-37b3-4fd7-9a08-fb8d1768ae5f/8ff1f92b-37b3-4fd7-9a08-fb8d1768ae5f.py

We could try to find a place where that is generated and add a line like logging.getLogger().setLevel(logging.WARNING) at the top of the file.

@hjtran
Copy link
Copy Markdown
Contributor Author

hjtran commented Mar 29, 2024

I attempted a fix to change the playground's #29948 but that didn't seem to take. Vlado Djerek also tried to figure out where the logging level is set but was also unsuccessful.

By crashing playground intepreter, I see that the main python entrypoint is located in some generated files, like: /opt/playground/backend/executable_files/8ff1f92b-37b3-4fd7-9a08-fb8d1768ae5f/8ff1f92b-37b3-4fd7-9a08-fb8d1768ae5f.py

We could try to find a place where that is generated and add a line like logging.getLogger().setLevel(logging.WARNING) at the top of the file.

I attempted this in another PR, though I don't actually know how to test it. I just removed the log level setting altogether, but we could change to just WARNING. IMO though, playground examples seem like an unlikely place to debug runner issues, and the examples are always easy to run locally anyways if we ever find something amiss.

@tvalentyn
Copy link
Copy Markdown
Contributor

I just removed the log level setting altogether, but we could change to just WARNING.

+1 to change to warning instead of removing

@tvalentyn tvalentyn closed this Apr 1, 2024
@tvalentyn tvalentyn reopened this Apr 1, 2024
@tvalentyn
Copy link
Copy Markdown
Contributor

(closed accidentally)

@tvalentyn
Copy link
Copy Markdown
Contributor

I also don't known how to test playground code. a log level change should be safe to merge.

@tvalentyn tvalentyn merged commit 2e630ac into apache:master Apr 5, 2024
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.

2 participants