fix(trainer): resolve E2E log streaming incompatibility with kubernetes client v35+#504
Conversation
…ubernetes client v35+ - Replace watch.Watch().stream() with response.stream() for pod log streaming - Pass _preload_content=False to read_namespaced_pod_log when following logs - Remove unused watch import that caused ApiTypeError in newer kubernetes versions - Update mock_read_namespaced_pod_log to support streaming responses - Add parameterized test case for get_job_logs with follow=True Fixes the RuntimeError in E2E tests: 'Got an unexpected keyword argument watch to method read_namespaced_pod_log' which occurs with kubernetes client >= 35.0.0 Signed-off-by: HKanoje <hrithik.kanoje@gmail.com>
|
[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 |
google-oss-prow
Bot
requested review from
Electronic-Waste,
kramaranya and
szaher
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates Kubernetes pod log streaming to use a streaming HTTP response (_preload_content=False) instead of watch.Watch().stream, and extends unit tests to cover follow=True behavior.
Changes:
- Switch
_read_pod_logs(..., follow=True)to stream viaread_namespaced_pod_log(..., _preload_content=False)andresponse.stream() - Add a streaming-capable mock for
read_namespaced_pod_login tests - Add a new test case validating
get_job_logs(..., follow=True)behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| kubeflow/trainer/backends/kubernetes/backend.py | Replaces Watch-based streaming with HTTPResponse streaming for follow=True pod logs. |
| kubeflow/trainer/backends/kubernetes/backend_test.py | Extends mocking/test cases to validate streaming logs via follow=True. |
| # Stream logs incrementally using response.stream(). | ||
| for line in response.stream(): | ||
| if line: | ||
| # Decode bytes to string and yield each line. | ||
| yield line.decode("utf-8").rstrip("\n") |
| # Stream logs incrementally using response.stream(). | ||
| for line in response.stream(): | ||
| if line: | ||
| # Decode bytes to string and yield each line. | ||
| yield line.decode("utf-8").rstrip("\n") |
| # Handle streaming case: when _preload_content=False and follow=True | ||
| if kwargs.get("_preload_content") is False and kwargs.get("follow") is True: | ||
| # Return a mock response object with a stream() method | ||
| mock_response = Mock() | ||
|
|
||
| def mock_stream(): | ||
| """Mock stream generator that yields log lines as bytes.""" | ||
| yield b"test log content" | ||
|
|
||
| mock_response.stream = mock_stream | ||
| return mock_response |
| TestCase( | ||
| name="valid flow with follow=True for streaming logs", | ||
| expected_status=SUCCESS, | ||
| config={"name": BASIC_TRAIN_JOB_NAME, "follow": True}, | ||
| expected_output=["test log content"], | ||
| ), |
HKanoje
changed the title
|
This issue is being tracked upstream in kubernetes python client - kubernetes-client/python#2596 (comment) |
|
There's no definite timeline yet for a patch release for the bug. There has been a recent 36.0.1 which has been breaking the tests again because it does not address the logging error. I suppose best to pin <36.0.0. |
Fix log streaming to properly yield line-terminated chunks instead of raw 64KB byte buffers, maintaining Iterator[str] contract for caller code.
Changes
Issue
Fixes the RuntimeError in E2E tests: 'Got an unexpected keyword argument watch to method read_namespaced_pod_log' which occurs with kubernetes client >= 35.0.0