nextflow-io / nextflow

A DSL for data-driven computational pipelines
http://nextflow.io
Apache License 2.0
2.76k stars 629 forks source link

Unnecessary copy of input files to the task work directory #961

Closed rsuchecki closed 5 years ago

rsuchecki commented 5 years ago

Bug report - or perhaps just an inconsistency

Expected behavior and actual behavior

As per documentation,

Some caveats on glob pattern behavior:

  • Input files are not included in the list of possible matches.

This no longer holds if scratch true directive is set for a process.

Steps to reproduce the problem

#!/usr/bin/env nextflow

Channel.from('Bonjour')
  .collectFile()
  .set { greetings }

process echo {
  scratch true
  input:
    file ('greeting.in')  from greetings

  output:
    file('*')

  script:
  """
  cat greeting.in greeting.in > greeting.out
  """
}
rm -rf work/ && nextflow run main.nf && tree -h work

Program output

With scratch false

File greetings.in is a symlink as expected.

N E X T F L O W  ~  version 18.10.1
Launching `main.nf` [hungry_easley] - revision: 99f69e3c7e
[warm up] executor > local
[27/82d2c1] Submitted process > echo (1)
work
├── [4.0K]  27
│   └── [4.0K]  82d2c16f3fcc6f85b8195c0801ba57
│       ├── [  71]  greeting.in -> /tmp/hello/work/tmp/2c/711ca50bd2a4c3e3ac1b437bb769c9/collect-file.data
│       └── [  14]  greeting.out
└── [4.0K]  tmp
    └── [4.0K]  2c
        └── [4.0K]  711ca50bd2a4c3e3ac1b437bb769c9
            └── [   7]  collect-file.data

With scratch true

File greetings.in is a regular file, in a real case scenario this could be a large file duplicated as many times as process is executed.

N E X T F L O W  ~  version 18.10.1
Launching `main.nf` [mad_minsky] - revision: abe1eb2016
[warm up] executor > local
[35/8379d6] Submitted process > echo (1)
work
├── [4.0K]  35
│   └── [4.0K]  8379d6f078b84b19445786988c69e9
│       ├── [   7]  greeting.in
│       └── [  14]  greeting.out
└── [4.0K]  tmp
    └── [4.0K]  73
        └── [4.0K]  2c980de793155629eabb0faca75422
            └── [   7]  collect-file.data

Environment

rsuchecki commented 5 years ago

By the way, this is specific to the default stageOutMode 'copy' but not stageOutMode 'move' or stageOutMode 'rsync', so perhaps cp should not be used with -L, --dereference?

pditommaso commented 5 years ago

Many things, let's focus on the issue subject. I've modified the test case as

Channel.from('Bonjour')
  .collectFile()
  .set { greetings }

process echo {
  input:
    file ('greeting.in')  from greetings

  output:
    file('*') into result

  script:
  """
  cat greeting.in > greeting.out
  """
}

result.println()

It output on file independently the use os scratch:

 $ nextflow run test.nf 
N E X T F L O W  ~  version 18.11.0-edge
Launching `test.nf` [stoic_hamilton] - revision: 1f7de396b7
[warm up] executor > local
[d8/93493b] Submitted process > echo (1)
/Users/pditommaso/projects/nextflow/work/d8/93493b90638693d394f44a2d321ad4/greeting.out

$ nextflow run test.nf -process.scratch true
N E X T F L O W  ~  version 18.11.0-edge
Launching `test.nf` [fervent_bassi] - revision: 1f7de396b7
[warm up] executor > local
[4e/d435a7] Submitted process > echo (1)
/Users/pditommaso/projects/nextflow/work/4e/d435a754f967b44eec35c914c2bfa2/greeting.out

Therefore the the assertion Input files are not included in the list of possible matches is valid in both cases.

rsuchecki commented 5 years ago

Thanks @pditommaso - I misdiagnosed the issue and (surprise, surprise) you are right about the main point so this issue should be closed as it is misleading.

I guess the actual concern is the fact that input file symlinks are being resolved when copying from scratch. If the process in question is executed multiple times, we'll end up with multiple copies of input files.

pditommaso commented 5 years ago

Damn! you are right, I'm getting what you mean, the problem is not the input file is copied from the work dir to the scratch but the other way around.

Now the problem is how to exclude from a cp * /target a list of known file name?

rsuchecki commented 5 years ago

Correct, it is about files being copied from scratch to work dir.

As for how - have to be careful, in simple terms this should work:

  1. resolve glob(s)
  2. filter out staged-in filenames
  3. Add any explicitly declared output filenames to capture inputs which are meant to be passed through the process (this may include one or more of the original inputs)
  4. cp...

but things could get tricky if multiple outputs specified especially in sub-dirs (?)

Exclusion would be ideal, as current set-up leads to a range of different outcomes depending on combination of settings used:

stageIn stageOut comment
copy any inputs captured by glob and copied/moved to work dir :x:
link move hardlink preserved :heavy_check_mark:
link cp/rsync hardlink lost :x:
symlink mv/rsync soft link preserved :heavy_check_mark:
symlink cp soft link resolved :x:, should suffice to run cp * /target without -L, --dereference but risky as symliks may be created within some dir structure generated by process script
rellink ignore, not allowed with scratch true

So the cases with :heavy_check_mark: indicate that although files are captured by glob, these are (sym/hard) links so additional consumption of disk space is not an issue.

As a separate point, can hard-links be of much use in the context of proper scratch space? That would typically be a separate file system?

pditommaso commented 5 years ago

Sorry for the very late reply, staring from point 1. resolve glob(s) not so trivial.

The main problem is that the glob can only resolved at runtime (a task can create any file) therefore the it must be resolved by the Bash script. Ideally it would needed a kind of cp <glob> /target --exclude <input file names list>.

rsuchecki commented 5 years ago

That is right, but glob can be resolved in bash before cp is called. For example, use ls or find to match the glob pattern, then filter out inputs using a temp file of filenames, or as below, process substitution to have one less file to worry about

ls GLOB | grep -vFf <(echo ${InputFileNameList}) | xargs cp --target-directory /target

(where InputFileNameList is the NF variable)

Also interesting: extended globbing

Side note: the rsync version should be straightforward with --include and --exclude.

pditommaso commented 5 years ago

Nice the extended globs, tho not sure it cal help in the case. I'm more thing something like already implement for Batch

https://github.com/nextflow-io/nextflow/blob/8932eb7903950b96401c19a8d2c2a606eb10ed21/src/main/groovy/nextflow/cloud/aws/batch/S3Helper.groovy#L34-L41

Evan the output file name glob in a for loop, then check for each entry if matches an input file name (or a previous output glob), it matchers => skip, otherwise copy it.

To check the if a file name matches a glob it can be used compgen

rsuchecki commented 5 years ago

Yes that looks ok, just a minor point - wondering if eval is needed here at all,

for name in \$(eval "ls -1d \$pattern");do 

should be equivalent to

for name in \$(ls -1d \$pattern);do 
pditommaso commented 5 years ago

I remember that I tried to not use eval but there was a problem, which frankly I'm unable to remind now.

pditommaso commented 5 years ago

I've mocked up a possible patch

all_patterns=('')

nxf_cp() {
    local pattern=$1
    local target=$2
    IFS=$'\n'
    for name in $(eval "ls -1d $pattern");do
       for var in "${all_patterns[@]}"; do [[ $name =~ $var ]] && continue 2; done
       cp $name $target
    done
    unset IFS
    all_patterns+=($pattern)
}
rsuchecki commented 5 years ago

I don't fully understand the context on how you would apply it (copy scratch to work? or first use it for copy from work to scratch, collecting the patterns to be excluded) but the mechanism appears to be sound, and if e.g. input file patterns are given they will not be copied.

Just thought about a scenario that can be relevant to this topic. I already have cases where I declare some file as one of the inputs AND one of the outputs - sort of pass-through a process - no issues there I think, BUT what if someone does that, but also modifies such file? I don't think that would be reasonable thing to do but if possible, it would affect caching, and would introduce ambiguity into what is copied or not copied from scratch.

pditommaso commented 5 years ago

This is supposed to be used in the place of the cp commands to stage out tasks outputs from the scratch to the task work dir https://github.com/nextflow-io/nextflow/blob/07df38ec1c66447741906149f47aaec1dfc2a5eb/modules/nextflow/src/test/groovy/nextflow/executor/BashWrapperBuilderTest.groovy#L391-L396

Actually it should be extended to handle the command in a parametric manner so that it can use cp, mv or rsycn along the same line of this method.

Just thought about a scenario that can be relevant to this topic. I already have cases where I declare some file as one of the inputs AND one of the outputs - sort of pass-through a process - no issues there I think, BUT what if someone does that, but also modifies such file? I don't think that would be reasonable thing to do but if possible, it would affect caching, and would introduce ambiguity into what is copied or not copied from scratch.

Currently NF excludes all the input files names from the output set when you specify a wildcard. It doesn't if the output is punctual.

Therefore I think this method should be changed so that: 1) find all output file names that contains no wildcard 2) copy those files using the current mechanism 3) create a list of all input names and the above outputs and use as exclusion list 4) copies the remaining outputs including a wildcard with the above bash function.

pditommaso commented 5 years ago

A possible strategy I have identified to handle this consist on create a file listing all task staged inputs jut before the task execution, with something like:

ls -1a > .command.skip_files

Then use the following script to copy or more task outputs taking into account staged inputs that needs to be skipped


nxf_sync() {
    local cmd=($1)      # command to be executed
    local source=$2     # source path, it may use glob pattern
    local target=$3     # target directory 
    local skip_inputs=${4:-true}    # flag to enable the skipping of input file (true when source is a glob)
    local INPUT_FILES=.command.skip_files

    IFS=$'\n'
    # expand the source pattern and iterate over each matching file 
    for name in $(eval "ls -1d $source");do
      # skip if the file name is in the list of input files
      if $skip_inputs && grep "^$name$" $INPUT_FILES &>/dev/null; then continue; fi
      # compute the new file name 
      local newfile=$target/$name
      # create the target directory
      [[ ! -e $newfile ]] && mkdir -p $(dirname $newfile)
      # copy / move the file 
      ${cmd[*]} $name $newfile
      # append the copied file to the list of input file to avoid to process again if it's matched in a different source pattern
      echo $name >> $INPUT_FILES
    done
    unset IFS
}

However I think introducing this logic seems overkill to me. I think it would add more sense to put a warning in the documentation and eventually to solve in the future.

Jokendo-collab commented 5 years ago

@rsuchecki I am running Nextflow version 19.04.0 and it stops and when I check the slurn output error it shows this. What do you think is the problem? N E X T F L O W ~ version 19.04.0 Launching main.nf [maniac_payne] - revision: 807b6d97e9

rsuchecki commented 5 years ago

Can you be more specific @javanOkendo? Perhaps discuss on https://gitter.im/nextflow-io/nextflow?