Skip to content

Conversation

@abdulfatir
Copy link
Contributor

@abdulfatir abdulfatir commented Apr 16, 2025

Issue #, if available: fixes #286, also see aws/sagemaker-python-sdk#4945

Description of changes: This PR modifies logging utils such that the root logger behavior through basicConfig is not changed, rather the log level and handler are changed for the sagemaker_core.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@abdulfatir abdulfatir requested a review from a team as a code owner April 16, 2025 14:26
@abdulfatir abdulfatir requested a review from zhaoqizqwang April 16, 2025 14:26
@abdulfatir abdulfatir changed the title Fix log level change Fix logging behavior Apr 16, 2025
handler = get_rich_handler()
logging.basicConfig(level=getattr(logging, log_level), handlers=[handler])
logger = logging.getLogger(name)
logger.addHandler(handler)
Copy link
Contributor

Choose a reason for hiding this comment

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

would it make sense to clear all handlers before adding this one back? If this gets called twice could end up with duplicate logs being printed

for handler in logger.handlers:
	logger.removeHandler(handler)
logger.addHandler(handler)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, fixed!

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed style issue.

Copy link
Contributor

@benieric benieric Apr 16, 2025

Choose a reason for hiding this comment

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

I think may need to run black .

benieric
benieric previously approved these changes Apr 16, 2025
@abdulfatir
Copy link
Contributor Author

black issue is unrelated to this PR

@benieric benieric merged commit b006763 into aws:main Apr 16, 2025
13 of 14 checks passed
@abdulfatir abdulfatir deleted the fix-log-level branch April 16, 2025 21:04
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.

Why is sagemaker-core setting the log_level of the root logger to INFO?

2 participants