Fix interactive debugging bugs - #313
Open
JyotinderSingh wants to merge 3 commits into
Open
Conversation
The printed VS Code launch.json hardcoded "remoteRoot": "/tmp/workspace", which has not been where sources live since keras-team#222. The runner extracts the workspace under a mkdtemp(prefix="kinetic-run-") directory and symlinks the client's working_dir at it, so paths match on both sides and the mapping is the identity. Print that instead, and omit pathMappings entirely when the working directory is unknown (the `kinetic jobs debug` path), which is what debugpy already assumes. _require_interactive_terminal() ran after submit_remote(), so a blocking debug=True call without a TTY raised RuntimeError but left a job on the cluster that waited out the whole attach window and then ran anyway. Check before submitting. run_async() is unaffected and still works headless. The test now asserts submit_remote is never called; previously it only checked debug_attach, which passed even while a job was being submitted. KINETIC_DEBUG_WAIT_TIMEOUT was documented as user-facing but nothing read it on the client: the attach window was a hardcoded constant that the backends copied into the pod. Resolve it on the client instead, and use it for both the local wait and the pod env var. Pathways resolves once so the leader and its workers cannot disagree, which matters because workers wait the leader's window plus a buffer. Invalid values fall back to 10 minutes with a warning rather than disabling the wait. Also correct stale references left behind: the docs still told users to replace ${workspaceFolder} in the printed snippet, JobHandle.debug_attach still documented a placeholder, and a runner comment named the deleted DEBUG_WAIT_TIMEOUT constant.
The prose added with the previous commit did not follow the house
style. It used unapproved words ("thus", "assumes"), an idiom
("breakpoints hit the matching files"), "agree" in a non-literal sense,
and a colon splice. Rewrite the two changed sections as short active
present-tense sentences, and reuse the terms that troubleshooting.md
already uses for the same mechanism ("symbolic link", "client working
directory").
Also cover three facts the pages still left out:
- Automated environments: the TTY check now runs before submission,
and KINETIC_NO_TTY_DEBUG=1 overrides it. Drop the contractions.
- Multi-host: the leader and its workers take the same attach window,
so raising KINETIC_DEBUG_WAIT_TIMEOUT applies to every host.
- configuration.md: Kinetic reads the variable at submit time, so a
change after that has no effect on a job already in the cluster.
State the accepted range and the fallback.
JyotinderSingh
requested review from
divyashreepathihalli and
jeffcarp
as code owners
August 15, 2026 18:54
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #313 +/- ##
=======================================
Coverage ? 36.68%
=======================================
Files ? 84
Lines ? 15353
Branches ? 0
=======================================
Hits ? 5633
Misses ? 9720
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the remote debugging timeout and path mapping logic in Kinetic. It introduces resolve_debug_wait_timeout() to dynamically parse the KINETIC_DEBUG_WAIT_TIMEOUT environment variable on both the client and pod sides, falling back to a default of 10 minutes if invalid. It also updates VS Code launch.json generation to omit path mappings when the working directory is unknown and to safely escape paths using json.dumps. Additionally, the interactive terminal check for blocking debug runs is moved to execute before job submission to prevent orphaned jobs on the cluster. The feedback suggests converting working_dir using os.fspath before passing it to json.dumps to support pathlib.Path objects.
print_attach_instructions() passed working_dir straight to json.dumps, which raises TypeError on a pathlib.Path. debug_attach() is public API, so a caller can reasonably hand it a Path. The raise lands badly: print_attach_instructions() runs after start_port_forward(), so debug_attach() aborts with kubectl already running and returns no handle to clean it up. The user loses the session and leaks the subprocess. Convert with os.fspath first, and widen the debug_attach annotation to str | os.PathLike[str] | None so the signature matches what the function accepts. Also reframe the escaping test. Escaping is not a Windows concern: POSIX allows a backslash and a double quote in a directory name, and either one breaks the snippet the user pastes into launch.json. Cover all three cases.
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
2 participants
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.
Description
The printed VS Code launch.json hardcoded
"remoteRoot": "/tmp/workspace", which has not been where sources live since #222. The runner extracts the workspace under amkdtemp(prefix="kinetic-run-")directory and symlinks the client'sworking_dirat it, so paths match on both sides and the mapping is the identity. Print that instead, and omitpathMappingsentirely when the working directory is unknown (thekinetic jobs debugpath), which is what debugpy already assumes._require_interactive_terminal()ran aftersubmit_remote(), so a blockingdebug=Truecall without a TTY raisedRuntimeErrorbut left a job on the cluster that waited out the whole attach window and then ran anyway. Check before submitting.run_async()is unaffected and still works headless. The test now assertssubmit_remoteis never called; previously it only checkeddebug_attach, which passed even while a job was being submitted.KINETIC_DEBUG_WAIT_TIMEOUTwas documented as user-facing but nothing read it on the client: the attach window was a hardcoded constant that the backends copied into the pod. Resolve it on the client instead, and use it for both the local wait and the pod env var. Pathways resolves once so the leader and its workers cannot disagree, which matters because workers wait the leader's window plus a buffer. Invalid values fall back to 10 minutes with a warning rather than disabling the wait.Also correct stale references left behind: the docs still told users to replace
${workspaceFolder}in the printed snippet,JobHandle.debug_attachstill documented a placeholder, and a runner comment named the deletedDEBUG_WAIT_TIMEOUTconstant.Contributor Agreement