Ensure sibling files in toil-wdl-runner

[stxue1]

This will resolve #4595

Changelog Entry

To be copied to the draft changelog by merger:

• Ensure that sibling files are preserved between node uploads in toil-wdl-runner

Reviewer Checklist

• Make sure it is coming from issues/XXXX-fix-the-thing in the Toil repo, or from an external repo.
  • If it is coming from an external repo, make sure to pull it in for CI with:

    contrib/admin/test-pr otheruser theirbranchname issues/XXXX-fix-the-thing
    

  • If there is no associated issue, create one.
• Read through the code changes. Make sure that it doesn't have:
  • Addition of trailing whitespace.
  • New variable or member names in camelCase that want to be in snake_case.
  • New functions without type hints.
  • New functions or classes without informative docstrings.
  • Changes to semantics not reflected in the relevant docstrings.
  • New or changed command line options for Toil workflows that are not reflected in docs/running/{cliOptions,cwl,wdl}.rst
  • New features without tests.
• Comment on the lines of code where problems exist with a review comment. You can shift-click the line numbers in the diff to select multiple lines.
• Finish the review with an overall description of your opinion.

Merger Checklist

• Make sure the PR passes tests.
• Make sure the PR has been reviewed since its last modification. If not, review it.
• Merge with the Github "Squash and merge" feature.
  • If there are multiple authors' commits, add Co-authored-by to give credit to all contributing authors.
• Copy its recommended changelog entry to the Draft Changelog.
• Append the issue number in parentheses to the changelog entry.

[adamnovak]

Looks like the MiniWDL self test does not pass:

	Traceback (most recent call last):
	  File "/builds/databiosphere/toil/src/toil/worker.py", line 406, in workerScript
	    job._runner(jobGraph=None, jobStore=jobStore, fileStore=fileStore, defer=defer)
	  File "/builds/databiosphere/toil/src/toil/job.py", line 2811, in _runner
	    returnValues = self._run(jobGraph=None, fileStore=fileStore)
	  File "/builds/databiosphere/toil/src/toil/job.py", line 2728, in _run
	    return self.run(fileStore)
	  File "/builds/databiosphere/toil/src/toil/wdl/wdltoil.py", line 1428, in run
	    bindings = devirtualize_files(bindings, standard_library)
	  File "/builds/databiosphere/toil/src/toil/wdl/wdltoil.py", line 829, in devirtualize_files
	    return map_over_files_in_bindings(environment, stdlib._devirtualize_filename)
	  File "/builds/databiosphere/toil/src/toil/wdl/wdltoil.py", line 984, in map_over_files_in_bindings
	    return map_over_typed_files_in_bindings(bindings, lambda _, x: transform(x))
	  File "/builds/databiosphere/toil/src/toil/wdl/wdltoil.py", line 974, in map_over_typed_files_in_bindings
	    return environment.map(lambda b: map_over_typed_files_in_binding(b, transform))
	  File "/builds/databiosphere/toil/venv/lib/python3.9/site-packages/WDL/Env.py", line 151, in map
	    fb = f(b)
	  File "/builds/databiosphere/toil/src/toil/wdl/wdltoil.py", line 974, in <lambda>
	    return environment.map(lambda b: map_over_typed_files_in_binding(b, transform))
	  File "/builds/databiosphere/toil/src/toil/wdl/wdltoil.py", line 993, in map_over_typed_files_in_binding
	    return WDL.Env.Binding(binding.name, map_over_typed_files_in_value(binding.value, transform), binding.info)
	  File "/builds/databiosphere/toil/src/toil/wdl/wdltoil.py", line 1019, in map_over_typed_files_in_value
	    new_path = transform(value.type, value.value)
	  File "/builds/databiosphere/toil/src/toil/wdl/wdltoil.py", line 984, in <lambda>
	    return map_over_typed_files_in_bindings(bindings, lambda _, x: transform(x))
	  File "/builds/databiosphere/toil/src/toil/wdl/wdltoil.py", line 488, in _devirtualize_filename
	    file_id, parent_id, file_basename = unpack_toil_uri(filename)
	  File "/builds/databiosphere/toil/src/toil/wdl/wdltoil.py", line 334, in unpack_toil_uri
	    raise ValueError(f"Wrong number of path segments in URI: {toil_uri}")
	ValueError: Wrong number of path segments in URI: toilfile:77%3A0%3Afiles%2Ffor-job%2Fkind-WDLWorkflowNodeJob%2Finstance-5d763d7k%2Ffile-9c4f99f31c1e412382d40794ba45b968%2Ftmpatj37zsh//builds/databiosphere/toil/wdl-conformance-tests/tmpatj37zsh

[adamnovak]

It looks like the implementation here doesn't account for anything about a file except for its basename and its creating job. In particular, the other parts of the file path other than just the file's own name are ignored. So e.g. dir1/index.txt and dir2/index.txt from the same job will not both be able to be used by a subsequent job.

We might need to add a test for that and then fix the implementation to pass it.

[adamnovak]

It looks like we implicitly rely on self._file_store.localTempDir being the current directory here?

Probably we should construct the path to the directory we want to put the file in, make it, construct the file path from that, and return that.

[adamnovak]

Why not make the UUID mandatory in pack_toil_uri? If it should always be there, we probably want to actually enforce that. We could also move all the type conversion into the pack/unpack functions, so only real UUID objects go in and out.

[adamnovak]

So the algorithm is to set up all the files from a particular previous job in a single host directory, under their basenames, and then mount them into the container so that everything in a given host directory stays together.

That definitely will mostly work, but it won't handle the case where multiple files with the same basename, but different paths, are uploaded by a job.

I can have a job make dir1/index.txt and dir2/index.txt as outputs, and use them both as inputs to another job. It looks like with the current implementation one of those is going to overwrite the other, since they have the same creator UUID and the same basename.

[adamnovak]

I feel like it might still be possible to defeat this, but it's definitely better and should work correctly for most non-adversarial workflows.

[adamnovak]

@stxue1 If we don't pass the directory to ID mapping between the different standard library instantiations, is it possible to get two different standard library instances to upload two sibling files, but with different source directory UUIDs? Maybe by abusing the string-to-File logic?

[stxue1]

I think string-to-file logic will probably not support sibling files since the standard library instances might be different. The directory to ID mappings would probably have to be passed around in order to support this.