fix(trainer): fallback to previous pod logs on crash and detect OpenS…#47
fix(trainer): fallback to previous pod logs on crash and detect OpenS…#47reckless-sherixx wants to merge 3 commits into
Conversation
|
🎉 Welcome to the Kubeflow MCP Server! 🎉 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
Improves trainer log retrieval robustness for crash-looping/terminated pods and adds actionable diagnostics by pattern-matching common OpenShift permission and HuggingFace cache failures.
Changes:
- Add fallback to Kubernetes
read_namespaced_pod_log(..., previous=True)when Trainer SDK returns no active log lines. - Extend failure-hint extraction with OpenShift permission and HuggingFace cache error signatures.
- Add unit tests covering hint extraction and the
previous=Truefallback behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| kubeflow_mcp/trainer/api/monitoring.py | Adds failure-pattern matching and a Kubernetes previous-log fallback when SDK logs are empty. |
| kubeflow_mcp/trainer/api/sdk_contracts_test.py | Adds tests validating new failure-hint patterns and the previous-log fallback behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7d551c4 to
ecb1253
Compare
|
@abhijeet-dhumal Kindly review the pr i have made the changes suggested by copilot too |
Krishna-kg732
left a comment
Krishna-kg732
left a comment
There was a problem hiding this comment.
Hey @reckless-sherixx , thanks for working on this , overall looks good to me , just a tiny nit
cc : @abhijeet-dhumal
| hint2 = _extract_failure_hint("Permission denied when writing huggingface cache") | ||
| assert hint2 is not None and hint2["category"] == "HF_CACHE_WRITE_ERROR" | ||
|
|
||
| def test_get_training_logs_fallback_to_previous_logs(self): |
There was a problem hiding this comment.
Assert fallback trigger condition explicitly
In test_get_training_logs_fallback_to_previous_logs, ensure test verifies fallback happens only when get_job_logs returns empty.
|
@Krishna-kg732: changing LGTM is restricted to collaborators DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/lgtm Clean fix for crash-looped pod log retrieval. Overlaps partially with #46 — if both land, verify no duplicate error-detection paths. |
|
/ok-to-test |
|
@reckless-sherixx can you resolve merge conflicts ? |
Yeah sure I'll do that |
37c6779 to
4e127ee
Compare
|
[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 |
| ( | ||
| re.compile(r"Permission denied.*(/\.local|/home|/\.cache)", re.IGNORECASE), | ||
| "OPENSHIFT_PERMISSION_ERROR", | ||
| "OpenShift random UID cannot write to home directory; set env var HF_HOME=/workspace or mount a writable volume.", | ||
| ), |
There was a problem hiding this comment.
Hey @reckless-sherixx , Can you please check with @priyank766 's implementation of the same in #46
| "OpenShift random UID cannot write to home directory; set env var HF_HOME=/workspace or mount a writable volume.", | ||
| ), | ||
| ( | ||
| re.compile(r"Permission denied.*huggingface|HF_HOME", re.IGNORECASE), |
There was a problem hiding this comment.
@reckless-sherixx HF regex still seems broken
Can we put the HF pattern before the general OpenShift one so /.cache HF failures don’t become OPENSHIFT_PERMISSION_ERROR.
There was a problem hiding this comment.
something like this ? WDYT?
r"Permission denied.*(?:huggingface|HF_HOME)|(?:huggingface|HF_HOME).*Permission denied"
There was a problem hiding this comment.
And test coverage should track this correctly
|
@reckless-sherixx #46 just merged .. it lands OPENSHIFT_PIP_ERROR for Permission denied on /.local |
|
Ok I'll make the changes |
…hift/HF failure patterns Signed-off-by: Vidyansh <ezboi2312@gmail.com>
Signed-off-by: Vidyansh <ezboi2312@gmail.com>
…RROR (kubeflow#46 covers it), add comprehensive HF tests Signed-off-by: Vidyansh <ezboi2312@gmail.com>
4e127ee to
2862569
Compare
|
/lgtm thanks for working on this cc: @abhijeet-dhumal , @jaiakash |
Resolves #42
Description
When a training job's pods crash-loop or terminate (common on OpenShift environments due to random UID permission gotchas or HuggingFace cache errors),
get_training_logs()previously returned empty logs via the Trainer SDK because active container logs are flushed upon crash/restart.This PR implements:
previous=TrueFallback: Inget_training_logs(), if active container logs are empty, the server queriesCoreV1Api.read_namespaced_pod_log(..., previous=True)to recover the crash output from terminated container instances.Permission deniedon/.local,/home) and HuggingFace cache directory failures, returning actionable guidance (set HF_HOME=/workspace).Related Issue
Fixes #42
Checklist
make test-python)make verify)git commit -s)Testing
Added
test_extract_failure_hint_openshift_and_hfandtest_get_training_logs_fallback_to_previous_logsinsdk_contracts_test.pyverifying pattern extraction and mock pod log fallback.