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.

Sign up for GitHub

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

Jump to bottom

Add sdk_harness_log_level_overrides option for python sdk #22077

Merged
merged 7 commits into from
Jun 30, 2022
Merged

Add sdk_harness_log_level_overrides option for python sdk #22077

merged 7 commits into from
Jun 30, 2022

Conversation

Abacn
Copy link
Contributor

@Abacn Abacn commented Jun 28, 2022 •
edited
Loading

Fixes #18222

Please add a meaningful description for your change here

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests

See CI.md for more information about GitHub Actions CI.

@asf-ci
Copy link

asf-ci commented Jun 28, 2022

Can one of the admins verify this patch?

4 similar comments
@asf-ci
Copy link

asf-ci commented Jun 28, 2022

Can one of the admins verify this patch?

@asf-ci
Copy link

asf-ci commented Jun 28, 2022

Can one of the admins verify this patch?

@asf-ci
Copy link

asf-ci commented Jun 28, 2022

Can one of the admins verify this patch?

@asf-ci
Copy link

asf-ci commented Jun 28, 2022

Can one of the admins verify this patch?

@codecov
Copy link

codecov bot commented Jun 28, 2022 •
edited
Loading

Codecov Report

Merging #22077 (f855dda) into master (141e950) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #22077      +/-   ##
==========================================
- Coverage   74.00%   74.00%   -0.01%     
==========================================
  Files         703      703              
  Lines       92936    92955      +19     
==========================================
+ Hits        68774    68787      +13     
- Misses      22896    22902       +6     
  Partials     1266     1266
Flag Coverage Δ
python 83.58% <100.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
...dks/python/apache_beam/options/pipeline_options.py 94.96% <100.00%> (+0.01%) ⬆️
...thon/apache_beam/runners/worker/sdk_worker_main.py 78.36% <100.00%> (+2.54%) ⬆️
.../python/apache_beam/testing/test_stream_service.py 88.09% <0.00%> (-4.77%) ⬇️
sdks/python/apache_beam/utils/interactive_utils.py 95.12% <0.00%> (-2.44%) ⬇️
...che_beam/runners/interactive/interactive_runner.py 90.06% <0.00%> (-1.33%) ⬇️
...hon/apache_beam/runners/worker/bundle_processor.py 93.42% <0.00%> (-0.13%) ⬇️
sdks/python/apache_beam/transforms/core.py 92.57% <0.00%> (ø)
...thon/apache_beam/ml/inference/pytorch_inference.py 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 141e950...f855dda. Read the comment docs.

@Abacn Abacn marked this pull request as ready for review June 28, 2022 15:42
@Abacn
Copy link
Contributor Author

Abacn commented Jun 28, 2022

R: @tvalentyn

@github-actions
Copy link
Contributor

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

tvalentyn
tvalentyn reviewed Jun 29, 2022
View reviewed changes
sdks/python/apache_beam/options/pipeline_options.py Outdated
'For example, by specifying the value "{\"a.b.c\":\"DEBUG\"}, '
'the logger underneath the module "a.b.c" will be configured to '
'output logs at the DEBUG level. Similarly, by specifying the '
'value {"a.b.c":"WARNING"} all loggers underneath the "a.b.c" '
Copy link
Contributor

Choose a reason for hiding this comment

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

is escaping quotes necessary here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, not necessary. Copied from java sdk and found I even missed a quote mark. Will fix

tvalentyn
tvalentyn approved these changes Jun 29, 2022
View reviewed changes
sdks/python/apache_beam/runners/worker/sdk_worker_main.py Outdated
parsed_overrides.update(line)
except Exception:
_LOGGER.error(
"Unable to parse sdk_harness_log_level_overrides. "
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe print the line that cannot be parsed here?

sdks/python/apache_beam/runners/worker/sdk_worker_main.py Outdated Show resolved Hide resolved
sdks/python/apache_beam/runners/worker/sdk_worker_main_test.py Outdated Show resolved Hide resolved
Abacn and others added 2 commits June 29, 2022 11:01
@Abacn @tvalentyn
Apply suggestions from code review
2b01b1b 
Co-authored-by: tvalentyn <tvalentyn@users.noreply.github.com>
@Abacn
Fix quptes; print option in error msg
b794767
@github-actions
Copy link
Contributor

The Workflow run is cancelling this PR. It is an earlier duplicate of 1729654 run.

@github-actions
Copy link
Contributor

The Workflow run is cancelling this PR. It is an earlier duplicate of 2083803 run.

@Abacn
Copy link
Contributor Author

Abacn commented Jun 30, 2022 •
edited
Loading

@tvalentyn PTAL

@tvalentyn tvalentyn merged commit d6b6253 into apache:master Jun 30, 2022
@tvalentyn
Copy link
Contributor

Thanks a lot, @Abacn , very nice.

@Abacn Abacn deleted the logoverride branch June 30, 2022 19:57
@Abacn Abacn mentioned this pull request Feb 23, 2023
Closed
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tvalentyn tvalentyn tvalentyn approved these changes

Assignees
No one assigned
Labels
python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve Logging Capabilities
3 participants
@Abacn @asf-ci @tvalentyn