WIP: Vine Shared Filesystem Access via Symlinks #3976
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new symlink capability to TaskVine, allowing tasks to create symlinks within their sandbox namespace for accessing shared filesystem resources. The implementation replaces the previous symlink flag behavior (now uses hardlink instead) and introduces dedicated symlink functionality that bypasses TaskVine's caching infrastructure.
Key changes:
- Introduces
vine_symlinkstructure and related functions for managing symlinks - Adds
add_symlinkmethod to tasks for creating arbitrary symlinks in task sandboxes - Changes default file mounting behavior to use symlinks instead of hardlinks, with explicit hardlink flag for when needed
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| taskvine/test/vine_python.py | Updates test to use hardlink flag and adds new symlink test case |
| taskvine/src/worker/vine_worker.c | Adds protocol parsing for symlink commands |
| taskvine/src/worker/vine_sandbox.c | Implements symlink creation in sandbox staging and reverses mount behavior |
| taskvine/src/manager/vine_task.h | Adds symlink_list field to task structure |
| taskvine/src/manager/vine_task.c | Implements symlink list management and adds vine_task_add_symlink function |
| taskvine/src/manager/vine_symlink.h | Defines vine_symlink structure and function declarations |
| taskvine/src/manager/vine_symlink.c | Implements vine_symlink creation, copying, and deletion functions |
| taskvine/src/manager/vine_manager_put.c | Adds protocol generation for sending symlink commands to workers |
| taskvine/src/manager/taskvine.h | Updates API documentation and mount flag definitions |
| taskvine/src/manager/Makefile | Adds vine_symlink.c to build sources |
| taskvine/src/bindings/python3/ndcctools/taskvine/task.py | Updates Python bindings to use hardlink flag instead of symlink flag |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| t.add_symlink("input","infile" ) | ||
| t.add_symlink("output","outfile" ) |
There was a problem hiding this comment.
The symlink targets "infile" and "outfile" appear to be undefined variables or paths. These should be valid absolute paths to existing files in the shared filesystem for the symlinks to work correctly.
| t.add_symlink("input","infile" ) | |
| t.add_symlink("output","outfile" ) | |
| t.add_symlink("input", input_file) | |
| t.add_symlink("output", path.join(test_dir, "outs", output_name)) |
Copilot uses AI. Check for mistakes.
| result = symlink(s->target, s->name); | ||
| if (result != 0) { | ||
| debug(D_VINE, "unable to symlink %s -> %s: %s", s->name, s->target, strerror(errno)); | ||
| break; | ||
| } |
There was a problem hiding this comment.
The success condition check is incorrect. The symlink() function returns 0 on success and -1 on failure. The current logic treats success (0) as failure and will break the loop on the first successful symlink creation.
Copilot uses AI. Check for mistakes.
|
|
||
| #include "vine_symlink.h" |
There was a problem hiding this comment.
Missing copyright header. The file should include the standard copyright notice consistent with other files in the codebase.
Copilot uses AI. Check for mistakes.
| raise TypeError(f"remote_name {remote_name} is not a str") | ||
|
|
||
| flags = Task._determine_mount_flags(strict_input=strict_input, mount_symlink=mount_symlink) | ||
| flags = Task._determine_mount_flags(strict_input=strict_input,mount_hardlink=mount_hardlink) |
There was a problem hiding this comment.
[nitpick] Missing space after comma in function call. Should be mount_hardlink=mount_hardlink with proper spacing.
| flags = Task._determine_mount_flags(strict_input=strict_input,mount_hardlink=mount_hardlink) | |
| flags = Task._determine_mount_flags(strict_input=strict_input, mount_hardlink=mount_hardlink) |
Copilot uses AI. Check for mistakes.
| # >>> task.add_input(f,"data") | ||
| # @endcode | ||
| def add_input(self, file, remote_name, strict_input=False, mount_symlink=False): | ||
| def add_input(self, file, remote_name, strict_input=False, mount_hardlink=False ): |
There was a problem hiding this comment.
[nitpick] Extra space before the closing parenthesis. Should be mount_hardlink=False) without the trailing space.
| def add_input(self, file, remote_name, strict_input=False, mount_hardlink=False ): | |
| def add_input(self, file, remote_name, strict_input=False, mount_hardlink=False): |
Copilot uses AI. Check for mistakes.
Proposed Changes
Add capability to add symlink to task sandbox namespace.
Will then add mode to substitute regular file objects with symlinks to shared filesystem.
Merge Checklist
The following items must be completed before PRs can be merge.
Check these off to verify you have completed all steps.
make testRun local tests prior to pushing.make formatFormat source code to comply with lint policies. Note that some lint errors can only be resolved manually (e.g., Python)make lintRun lint on source code prior to pushing.