nextflow
nextflow copied to clipboard
nextflow-io
[Bugfix] Fix broken symlinks with scratch and move
In an old version of nf-core/viralrecon I found the following code:
https://github.com/nf-core/viralrecon/blob/75a2f9763e9b4c1aa20ad342ae3ce148c33cc65a/main.nf#L660-L661
Creating symlinks leads to problems if the workflow is executed in a scratch directory, moving the outputs.
This PR solves this issue.
To test my fix, I run the following script, which fails with the current Nextflow version:
nextflow run test.nf -process.scratch true -process.stageOutMode move
...
Error executing process > 'aGlobal (1)'
Caused by:
Missing output file(s) `file.txt` expected by process `aGlobal (1)`
Command executed:
ln -s test.txt file.txt
Command exit status:
0
Command output:
(empty)
Script:
process aLocal {
output:
file 'file.txt' into aOut1
"""
echo "This is the file" > original.txt
ln -s original.txt file.txt
"""
}
process aGlobal {
input:
file a from Channel.fromPath("test.txt")
output:
file 'file.txt' into aOut2
"""
ln -s $a file.txt
"""
}
process b {
input:
path a from aOut1.mix(aOut2)
"""
stat $a
"""
}
Looking forward to feedback.
Well spotted, it's worth adding a test to verify to track this behaviour. Usually this is a done adding a script in tests directory. The test script should match a directory with the same name in the checks subdirectory, which should contain the .checks Bash script which carry out the actual test
Hi @pditommaso, thank you for pointing out the tests. I published my test case. I rewrote my first suggestion, as it had problems if Nextflow had already moved the destination of a symlink. Moreover, this solution keeps the relative symlinks relative and absolute ones absolute - this seems more natural. Furthermore, if a folder is moved and contains symlinks, the symlinks are also resolved.
Hi @pditommaso, I updated the test case to DSL 2. Furthermore, I forgot to add -process.scratch true -process.stageOutMode move in the test case. I also added it now.
So the PR is ready to get merged. What do you think?
I've been using the changed method for four months now, and I have never faced any problem.
I think this is going beyond the expectation of what nextflow should do. If I'm understanding currently the problem is related to the copy or symlink, but this adds too much logic in the launcher script.
I'm inclined to not merge this.
Thanks a lot for your review, @pditommaso. This PR is required if you create a symlink in a task but only define the symlink as an output, and you then use the scratch and move option.
I see your point and thought about this. The logic was much easier in my first commit but would not catch all cases. In my opinion, it makes sense to keep the kind of symlink. However, I can revert these changes making it again like before. But this will not suit all test cases shown here:
https://github.com/nextflow-io/nextflow/blob/c59bce7043db1d1fe11470640a3b99ea7cc363df/tests/nxf_fs_move_symlinks.nf#L1-L106
Where I see more advantages: Until now, the nxf_fs_move/nxf_fs_copy methods are always added to the Bash Script. However, they are only required if process.scratch is activated and the mode is move/copy. So, I propose only adding these methods if needed. You also add complex code for tracing if the tracing is activated.
Not fixing this at all seems not the right way to me. Please tell me which way I should adjust this PR. I highly recommend the second one.
I have used the logic for months to execute different NF-Core workflows and never faced any issues.
It would not be enough to resolve symlink to the real file path when copying the file from the scratch path to the target one? think it's cp -L if I'm not wrong
Copying works fine, this only applies to: -process.scratch true -process.stageOutMode move.
In such cases, only the link is moved. You can run my test case with the current Nextflow Version.
This is problematic if the link is relative or absolute, but the destination file is in the scratch directory, which is deleted afterward.
I see, could not this snippet be used to resolve the link to the real path to keep the overall launcher code more compact
I do two things. First, I fetch the symlink, which is what your method does. https://github.com/nextflow-io/nextflow/blob/c59bce7043db1d1fe11470640a3b99ea7cc363df/modules/nextflow/src/main/resources/nextflow/executor/command-run.txt#L124
In the next steps, I try to keep the kind of the symlink. Four cases exists:
- Absolute Path pointing outside of the stage dir: Only move the symlink
- Absolute Path pointing inside the stage dir: Move the destination file + create a new symlink pointing to the new destination
- Relative symlink pointing outside of the stage dir: Create a new relative symlink in the destination directory
- Relative symlink pointing inside the stage dir: Move symlink + destination file.
A simpler approach would be to figure out the destination file, move it if it is inside the scratch folder, and create a relative or absolute symlink to it in the destination folder.
I think this was my approach in the first commit:
https://github.com/nextflow-io/nextflow/blob/915a8c257ba977518535a7c238bc23d4d87de5b2/modules/nextflow/src/main/resources/nextflow/executor/command-run.txt#L81-L103
Someone might assume to work with a relative or absolute path. But this might only apply to rare cases.