feat: add structured and configurable logging support to Kubeflow SDK#311
feat: add structured and configurable logging support to Kubeflow SDK#311MansiSingh17 wants to merge 3 commits intokubeflow:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
🎉 Welcome to the Kubeflow SDK! 🎉 Thanks for opening your first PR! We're happy to have you as part of our community 🚀 Here's what happens next:
Join the community:
Feel free to ask questions in the comments if you need any help or clarification! |
There was a problem hiding this comment.
Pull request overview
This PR adds structured logging support to the Kubeflow SDK by introducing a get_logger() utility function in kubeflow/common/logging.py that supports configurable log levels via the KUBEFLOW_LOG_LEVEL environment variable. The PR also replaces print() calls with logger.info() calls in two backend files to improve debugging and observability.
Changes:
- Added
kubeflow/common/logging.pywith aget_logger()function that provides custom formatting and environment variable-based log level configuration - Replaced
print()withlogger.info()inget_runtime_packages()methods of kubernetes and container backends
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| kubeflow/common/logging.py | New logging utility with configurable log levels via KUBEFLOW_LOG_LEVEL environment variable |
| kubeflow/trainer/backends/kubernetes/backend.py | Changed print() to logger.info() for job logs output |
| kubeflow/trainer/backends/container/backend.py | Changed print() to logger.info() for runtime package logs output |
9403274 to
c8efd98
Compare
- Add kubeflow/common/logging.py with get_logger() utility - Support configurable log levels via KUBEFLOW_LOG_LEVEL env variable - Replace print() calls with logger.info() in production code - Fixes kubeflow#85 Signed-off-by: Mansi Singh <singh.m1@northeastern.edu>
Signed-off-by: Mansi Singh <singh.m1@northeastern.edu>
c8efd98 to
99c60c8
Compare
|
It looks like the e2e test failure is related to a CI infrastructure issue with loading the deepspeed-runtime image into KinD, not related to the logging changes in this PR. Could a maintainer re-run the tests or confirm? |
|
@MansiSingh17 you're right. It should have been fixed in trainer now. /retest |
Signed-off-by: Mansi Singh <singh.m1@northeastern.edu>
|
@astefanutti, Fixed the ruff formatting and import ordering issues — all 14 checks are now passing. The only pending one is tide, which is waiting on lgtm/approved labels. Happy to address any other feedback! |
|
@MansiSingh17 thanks. /assign @szaher @kramaranya @andreyvelich |
Summary
Fixes #85
Changes
kubeflow/common/logging.pywith aget_logger()utility functionKUBEFLOW_LOG_LEVELenvironment variable (defaults to INFO)print()calls withlogger.info()in production code:kubeflow/trainer/backends/kubernetes/backend.pykubeflow/trainer/backends/container/backend.pyHow to test
Set log level via environment variable:
Notes
print()calls insideprint_packages()were intentionally left as-is since they run inside the training containerprint()calls in docstring examples were also left unchanged