Repository navigation
fix(trainer): yield log lines from ContainerBackend.get_job_logs - #940
Open
prabindersinghh wants to merge 1 commit into
Open
prabindersinghh wants to merge 1 commit into
prabindersinghh wants to merge 1 commit into
Conversation
…ob_logs get_job_logs is documented to return an iterator of log lines, and the Kubernetes backend yields one line per item. The container backend passed through whatever the Docker or Podman adapter returned: the whole log as a single string without follow, and raw stream frames with follow, which can split one line across items or merge several into one. Buffer the chunks and yield complete lines without the line terminator, so iterating over the logs behaves the same on every backend. Assisted-by: Claude Signed-off-by: Prabinder Singh <prabindersinghh@gmail.com>
Contributor
|
[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 |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
get_job_logsis documented to return an iterator of log lines, and the Kubernetes backend yields one line per item. The container backend passed through whatever the Docker or Podman adapter returned: the whole log as one string withoutfollow, and raw stream frames withfollow, which can split a line across items or merge several into one.This buffers the chunks in
ContainerBackend.get_job_logsand yields complete lines without the line terminator, so iterating over logs behaves the same on every backend. The change sits in the backend rather than the adapters, so Docker and Podman both get it.Error handling in this method (the
"Error getting logs: ..."line) is unchanged here and can be looked at separately.Regression coverage (
test_get_job_logs_yields_lines, for bothfollow=Falseandfollow=True):Validation:
uv run pytest -q kubeflow/trainer/backends/container/backend_test.py: 52 passedmake verify: passeduv run pytest -q kubeflow: 723 passedWhich issue(s) this PR fixes (optional, in
Fixes #<issue number>, #<issue number>, ...format, will close the issue(s) when PR gets merged):Fixes #938
Checklist:
Docs are not needed: this makes the container backend match the behaviour
get_job_logsalready documents.AI usage:
Claude was used to help implement the fix and the regression tests. I reviewed the changes, confirmed the new tests fail on main and pass with the fix, and ran the validation above. The commit carries an
Assisted-bytrailer.