fix(ssh): add optional remote_root auto-creation#590
Open
njzjz-bot wants to merge 1 commit intodeepmodeling:masterfrom
Open
fix(ssh): add optional remote_root auto-creation#590njzjz-bot wants to merge 1 commit intodeepmodeling:masterfrom
njzjz-bot wants to merge 1 commit intodeepmodeling:masterfrom
Conversation
Add a create_remote_root switch for SSHContext so DPDispatcher can recursively create the configured remote_root when users opt in. This preserves the current safe default while fixing setups where parent directories do not already exist. Also persist the new option through Machine.serialize(), add unit tests for recursive mkdir and config round-tripping, and document the new knob in the SSH examples. Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.4)
dosubot
bot
added
size:L
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #590 +/- ##
==========================================
- Coverage 49.59% 48.85% -0.74%
==========================================
Files 40 40
Lines 3958 3977 +19
==========================================
- Hits 1963 1943 -20
- Misses 1995 2034 +39 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
📝 WalkthroughWalkthroughThis PR introduces a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
dpdispatcher/contexts/ssh_context.py (2)
545-565: Add type hints to_mkdirmethod and consider logging suppressed errors.Two observations:
Missing type hints - Per coding guidelines, add type annotations:
Silent error suppression - The
OSErrorcatch (lines 553-554, 564-565) will silently ignore not just "directory exists" but also permission errors, disk full, etc. Consider logging at debug level when an exception occurs, or checkingerrno.EEXISTspecifically.Suggested type hints
- def _mkdir(self, remote_dir, recursive=False): + def _mkdir(self, remote_dir: str, recursive: bool = False) -> None: if not remote_dir: return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dpdispatcher/contexts/ssh_context.py` around lines 545 - 565, Add proper type annotations to the _mkdir method signature (e.g., remote_dir: str | None, recursive: bool) and return type (-> None) and update any internal variable hints if needed; then replace the broad silent OSError suppression in the non-recursive and recursive mkdir blocks (inside _mkdir, referencing self.sftp and pathlib.PurePosixPath) with targeted handling: check the exception errno (import errno) and only ignore errno.EEXIST, otherwise log the exception at debug or warn level via the module/class logger (do not raise for unexpected errors) so permission/disk errors are visible during debugging.
467-467: Add type hint forcreate_remote_rootparameter.Per coding guidelines, Python code should include type annotations for better maintainability.
Suggested type hint
def __init__( self, local_root, remote_root, remote_profile, clean_asynchronously=False, - create_remote_root=False, + create_remote_root: bool = False, *args, **kwargs, ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dpdispatcher/contexts/ssh_context.py` at line 467, Add a type annotation to the create_remote_root parameter (e.g., change it to create_remote_root: bool = False) where it appears so the function signature includes the type; ensure any needed typing imports are present and update the docstring/signature in the same function to reflect the boolean type if applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dpdispatcher/machine.py`:
- Around line 178-181: serialize() currently only adds clean_asynchronously when
the context object has that attribute, which omits the key for LocalContext and
breaks test_machine_argcheck; update serialize(self, if_empty_remote_profile:
bool = False) -> dict to always include "clean_asynchronously" (use
getattr(self.context, "clean_asynchronously", False) or similar) and also
include create_remote_root consistently (use getattr with a sensible default),
and add the requested type annotation to the serialize method signature so it
returns a dict with the optional if_empty_remote_profile boolean parameter; this
keeps tests expecting "clean_asynchronously": False working and satisfies the
typing guideline.
---
Nitpick comments:
In `@dpdispatcher/contexts/ssh_context.py`:
- Around line 545-565: Add proper type annotations to the _mkdir method
signature (e.g., remote_dir: str | None, recursive: bool) and return type (->
None) and update any internal variable hints if needed; then replace the broad
silent OSError suppression in the non-recursive and recursive mkdir blocks
(inside _mkdir, referencing self.sftp and pathlib.PurePosixPath) with targeted
handling: check the exception errno (import errno) and only ignore errno.EEXIST,
otherwise log the exception at debug or warn level via the module/class logger
(do not raise for unexpected errors) so permission/disk errors are visible
during debugging.
- Line 467: Add a type annotation to the create_remote_root parameter (e.g.,
change it to create_remote_root: bool = False) where it appears so the function
signature includes the type; ensure any needed typing imports are present and
update the docstring/signature in the same function to reflect the boolean type
if applicable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18987df2-4b74-4ec1-ba65-d4134a5952ae
📒 Files selected for processing (7)
doc/context.mddoc/getting-started.mddpdispatcher/contexts/ssh_context.pydpdispatcher/machine.pyexamples/machine/ssh_proxy_command.jsontests/test_argcheck.pytests/test_ssh_create_remote_root.py
Comment on lines
+178
to
+181
| if hasattr(self.context, "clean_asynchronously"): | ||
| machine_dict["clean_asynchronously"] = self.context.clean_asynchronously | ||
| if hasattr(self.context, "create_remote_root"): | ||
| machine_dict["create_remote_root"] = self.context.create_remote_root |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if LocalContext defines clean_asynchronously attribute
rg -n "clean_asynchronously" dpdispatcher/contexts/local_context.py
# Check if the test passes with current implementation
rg -nA5 "def test_machine_argcheck" tests/test_argcheck.pyRepository: deepmodeling/dpdispatcher
Length of output: 297
🏁 Script executed:
#!/bin/bash
# Get the full test_machine_argcheck test
sed -n '12,35p' tests/test_argcheck.pyRepository: deepmodeling/dpdispatcher
Length of output: 838
🏁 Script executed:
#!/bin/bash
# Get the serialize method in machine.py around lines 178-181 and show more context
sed -n '160,200p' dpdispatcher/machine.pyRepository: deepmodeling/dpdispatcher
Length of output: 1824
Serialization issue: LocalContext machines will not include clean_asynchronously in serialized output, but the test expects it.
The serialize() method conditionally includes clean_asynchronously only via hasattr(self.context, "clean_asynchronously"). However, LocalContext does not define this attribute, so for LocalContext machines the field will not be included in the serialized dict. Yet test_machine_argcheck expects "clean_asynchronously": False in the output for a LocalContext machine (line 29). This will cause the test to fail.
Either:
LocalContextneeds to defineclean_asynchronouslyas an attribute, or- The test expectation is incorrect
Additionally, the serialize() method lacks type hints. Per the coding guidelines, add proper type annotations: def serialize(self, if_empty_remote_profile: bool = False) -> dict:
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dpdispatcher/machine.py` around lines 178 - 181, serialize() currently only
adds clean_asynchronously when the context object has that attribute, which
omits the key for LocalContext and breaks test_machine_argcheck; update
serialize(self, if_empty_remote_profile: bool = False) -> dict to always include
"clean_asynchronously" (use getattr(self.context, "clean_asynchronously", False)
or similar) and also include create_remote_root consistently (use getattr with a
sensible default), and add the requested type annotation to the serialize method
signature so it returns a dict with the optional if_empty_remote_profile boolean
parameter; this keeps tests expecting "clean_asynchronously": False working and
satisfies the typing guideline.
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
1 participant
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.
Problem
mkdirforremote_root, so configurations fail when parent directories do not already exist.Change
create_remote_rootboolean forSSHContext; when enabled, DPDispatcher recursively createsremote_rootand the submission working directory.false) so mistyped remote paths still fail loudly.Machine.serialize(), add unit tests, and document it in the SSH docs/examples.Tests
./.tmp-test-venv/bin/python -m pytest tests/test_ssh_create_remote_root.py tests/test_argcheck.py -qCloses #436
Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.4)
Summary by CodeRabbit
New Features
Documentation
Tests