Use sentinel value to deal with optional coerced files #4994

stxue1 · 2024-06-28T01:28:26Z

This closes #4988

Changelog Entry

To be copied to the draft changelog by merger:

toil-wdl-runner will not immediately error on nonexistent coerced files until outputted
- File? type for string to file coercion is now supported (will be nullified)

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.

(may want to remove monkeypatching entirely)

library for task outputs

…ed-files

…github.com:DataBiosphere/toil into issues/4988-defer-virtualization-for-coerced-files

adamnovak

This looks pretty good, but I think it needs more documentation for the new parameters it adds. I also am concerned we might be missing a virtualization pass that we still need. (So some high-level documentation in a long comment about what our general plan is with regard to when files are or are not local paths, and when they are or are not null, and when they are or are not the magic sentinel URIs, might be useful.)

adamnovak · 2024-07-02T17:37:28Z

src/toil/wdl/wdltoil.py

@@ -622,6 +624,8 @@ def evaluate_output_decls(output_decls: List[WDL.Tree.Decl], all_bindings: WDL.E
    output_bindings: WDL.Env.Bindings[WDL.Value.Base] = WDL.Env.Bindings()
    for output_decl in output_decls:
        output_value = evaluate_decl(output_decl, all_bindings, standard_library)
+        drop_if_missing_with_workdir = partial(drop_if_missing, work_dir=getattr(standard_library, "_execution_dir"))


Maybe we want to make standard_library a ToilWDLStdLibBase so we don't need getattr.

Also, it starts with _ and so maybe isn't really meant to be accessed here; maybe we want to be calling a method that gets this instead? Or maybe we want to rename the attribute if we need it outside the stdlib class itself?

It would probably be better if all standard_library's were typed as ToilWDLStdLibBase; this was mainly done for mypy as mypy thinks the type is WDL.StdLib.Base and not ToilWDLStdLibBase.

I can probably turn the gettr into a method, although this current logic may not be sufficient. Another issue I found is that this will coerce all nonexistent files to null, even if the specified type is not optional. For example:

... output { File f = "nonexistent.txt" } ...

This wasn't an issue previously as we ensured file existence at type coercion.

This issue actually is #5006

adamnovak · 2024-07-02T17:39:24Z

src/toil/wdl/wdltoil.py

+    def __init__(self, file_store: AbstractFileStore, execution_dir: Optional[str] = None, enforce_nonexistence: bool = True):
        """
        Set up the standard library.



What does enforce_nonexistence actually do? Does it cause things to not exist? What happens if we don't enforce it?

I find the name a bit confusing so it definitely needs some documentation.

adamnovak · 2024-07-02T17:40:00Z

src/toil/wdl/wdltoil.py

+                        enforce_nonexistence: bool = True) -> str:
        """
        Download or export a WDL virtualized filename/URL to the given directory.


We would also want to document enforce_nonexistence in this docstring.

adamnovak · 2024-07-02T17:43:26Z

src/toil/wdl/wdltoil.py

@@ -888,6 +899,7 @@ def devirtualize_to(filename: str, dest_dir: str, file_source: Union[AbstractFil
            logger.debug("Virtualized file %s is already a local path", filename)

        if not os.path.exists(result):
+            # Devirtualizing an unvirtualized file means the file is coerced from a string and never used/virtualized


I think the real point of this block is to catch cases where such a string has made it from one context to another without going through the virtualization/devirtualization that would ensure the file it refers to is actually available too.

Do we now expect to be passing a bunch of local paths to the devirtualize function? If we mostly expect to hit this block when we try to access a string filename that doesn't exist, more than we expect to hit it when a file has escaped its node without being virtualized, then maybe we want to change the resulting error message?

I think this comment was carried through accidentally from a previous implementation before using sentinel values.

adamnovak · 2024-07-02T17:48:35Z

src/toil/wdl/wdltoil.py

+def drop_if_missing(value_type: WDL.Type.Base, filename: str, work_dir: str) -> Optional[str]:
+    """
+    Return None if a file doesn't exist, or its path if it does.
+    """


The docstring here should document value_type and work_dir. It seems like maybe the filename is assumed to belong to a WDL value of the given type (which might be File or File?), and that relative paths are interpreted relative to work_dir, but that should all be in the docstring.

Also probably filename should be the first argument, since that's what we're really operating on, and then value_type which is extra information about this file for error reporting, and finally work_dir since that's environment-level information that would stay the same over multiple calls (and could even sensibly have a default value of ".").

I guess we got this argument order from map_over_typed_files_in_bindings, so maybe we just need to leave it. Or we could change the order map_over_typed_files_in_bindings passes things in.

Changing the argument order of map_over_typed_files_in_value may require tracking down all transform functions, so maybe I'll leave the current argument order. Maybe there's a reason those transform functions wanted the type first?

adamnovak · 2024-07-02T17:53:41Z

src/toil/wdl/wdltoil.py

-        run_job = WDLTaskJob(self._task, virtualize_files(bindings, standard_library), virtualize_files(runtime_bindings, standard_library), self._task_id, self._namespace, self._task_path, cores=runtime_cores or self.cores, memory=runtime_memory or self.memory, disk=runtime_disk or self.disk, accelerators=runtime_accelerators or self.accelerators, wdl_options=self._wdl_options)
+        run_job = WDLTaskJob(self._task, bindings, runtime_bindings, self._task_id, self._namespace, self._task_path, cores=runtime_cores or self.cores, memory=runtime_memory or self.memory, disk=runtime_disk or self.disk, accelerators=runtime_accelerators or self.accelerators, wdl_options=self._wdl_options)


Don't we still need to virtualize here? What if one of the task input defaults or body decls is like File the_file = write_lines(["a"]) and then we want to use its path in the command? It needs to get shipped from the job that evaluated the decls to the job that runs the command.

adamnovak · 2024-07-02T17:55:00Z

src/toil/wdl/wdltoil.py

+        # Since we process nonexistent files in WDLTaskWrapperJob as those must be run locally, don't try to devirtualize them
+        standard_library = ToilWDLStdLibBase(file_store, enforce_nonexistence=False)


This sounds like enforce_nonexistence might be the wrong name; it really is enforcing existence or detecting nonexistence as an error.

adamnovak · 2024-07-02T17:59:47Z

src/toil/wdl/wdltoil.py

+def monkeypatch_coerce(standard_library: ToilWDLStdLibBase, null_nonexistent_files: bool = False) -> Generator[None, None, None]:
    """
    Monkeypatch miniwdl's WDL.Value.Base.coerce() function to virtualize files when they are represented as Strings.


We probably should document null_nonexistent_files here. What's the default behavior for nonexistent files when it isn't set?

I think I forgot to remove this from an earlier implementation.

adamnovak · 2024-07-02T18:04:52Z

src/toil/wdl/wdltoil.py

@@ -574,6 +575,7 @@ def recursive_dependencies(root: WDL.Tree.WorkflowNode) -> Set[str]:
 # in the same destination directory, when dealing with basename conflicts.

 TOIL_URI_SCHEME = 'toilfile:'
+TOIL_NONEXISTENT_URI_SCHEME = 'nonexistent:'


We should explain our clever plan of using a whole bunch of different nonexistent:blah URIs to represent distinct nonexistent files somewhere, including some kind of reference back to the WDL spec for why we need it. Maybe here?

Otherwise people will see this and scratch their heads and be tempted to simplify it down to just None or a single nonexistent sentinel value, since they won't know why that can't work.

…-for-coerced-files' into issues/4988-defer-virtualization-for-coerced-files

adamnovak

I think my comments have been addressed.

adamnovak · 2024-07-03T20:23:07Z

src/toil/wdl/wdltoil.py

+    filename represents a URI or file name belonging to a WDL value of type value_type. work_dir represents
+    the current working directory of the job and is where all relative paths will be interpreted from


These could be :param: directives.

adamnovak · 2024-07-03T20:24:02Z

src/toil/wdl/wdltoil.py

-    map_over_files_in_bindings(environment, lambda x: paths.append(x))
+
+    def append_to_paths(path: str) -> Optional[str]:
+        # Append element and return the element. This is to avoid a logger warning inside map_over_typed_files_in_value()


Maybe we should just stop issuing that warning if we're having to return values we don't care about just to avoid it?

…ed-files

stxue1 and others added 10 commits June 25, 2024 20:32

Don't immediately virtualize

bed26af

(may want to remove monkeypatching entirely)

Try sentinel implementation

b536cba

add abstracted function and remove duplicated execution_dir in standard

c619590

library for task outputs

fix typo and satisfy mypy with getattr

fa62076

Merge branch 'master' into issues/4988-defer-virtualization-for-coerc…

bf2b876

…ed-files

Fix instance type checks as we were checking WDL types to WDL values

18bfd8b

Merge branch 'issues/4988-defer-virtualization-for-coerced-files' of …

86682e5

…github.com:DataBiosphere/toil into issues/4988-defer-virtualization-for-coerced-files

Merge master into issues/4988-defer-virtualization-for-coerced-files

77be45e

Merge master into issues/4988-defer-virtualization-for-coerced-files

ec0d253

Merge master into issues/4988-defer-virtualization-for-coerced-files

81eb164

adamnovak requested changes Jul 2, 2024

View reviewed changes

stxue1 added 3 commits July 3, 2024 12:44

Add documentation, fix variable names, remove unused

4b621bf

Missing type annotation

614f851

Merge remote-tracking branch 'origin/issues/4988-defer-virtualization…

21c7948

…-for-coerced-files' into issues/4988-defer-virtualization-for-coerced-files

adamnovak approved these changes Jul 3, 2024

View reviewed changes

adamnovak and others added 6 commits July 12, 2024 13:46

Merge branch 'master' into issues/4988-defer-virtualization-for-coerc…

1077da5

…ed-files

Merge master into issues/4988-defer-virtualization-for-coerced-files

b615d7d

Merge master into issues/4988-defer-virtualization-for-coerced-files

8be79fd

Merge master into issues/4988-defer-virtualization-for-coerced-files

030c7e0

Merge master into issues/4988-defer-virtualization-for-coerced-files

ef02825

Merge master into issues/4988-defer-virtualization-for-coerced-files

3dc096e

stxue1 merged commit 2ba2ba7 into master Jul 19, 2024
1 check passed

stxue1 deleted the issues/4988-defer-virtualization-for-coerced-files branch July 19, 2024 23:44

adamnovak mentioned this pull request Jul 22, 2024

Symlink pasthrough on WDL is broken again #5031

Closed

stxue1 mentioned this pull request Jul 24, 2024

Move file virtualization in toil-wdl-runner to task boundaries #5028

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use sentinel value to deal with optional coerced files #4994

Use sentinel value to deal with optional coerced files #4994

stxue1 commented Jun 28, 2024

adamnovak left a comment

adamnovak Jul 2, 2024

stxue1 Jul 2, 2024

stxue1 Jul 3, 2024 •

edited

Loading

adamnovak Jul 2, 2024

adamnovak Jul 2, 2024

adamnovak Jul 2, 2024

stxue1 Jul 3, 2024

adamnovak Jul 2, 2024

stxue1 Jul 3, 2024

adamnovak Jul 2, 2024

adamnovak Jul 2, 2024

adamnovak Jul 2, 2024

stxue1 Jul 3, 2024

adamnovak Jul 2, 2024

adamnovak left a comment

adamnovak Jul 3, 2024

adamnovak Jul 3, 2024

		run_job = WDLTaskJob(self._task, virtualize_files(bindings, standard_library), virtualize_files(runtime_bindings, standard_library), self._task_id, self._namespace, self._task_path, cores=runtime_cores or self.cores, memory=runtime_memory or self.memory, disk=runtime_disk or self.disk, accelerators=runtime_accelerators or self.accelerators, wdl_options=self._wdl_options)
		run_job = WDLTaskJob(self._task, bindings, runtime_bindings, self._task_id, self._namespace, self._task_path, cores=runtime_cores or self.cores, memory=runtime_memory or self.memory, disk=runtime_disk or self.disk, accelerators=runtime_accelerators or self.accelerators, wdl_options=self._wdl_options)

		# Since we process nonexistent files in WDLTaskWrapperJob as those must be run locally, don't try to devirtualize them
		standard_library = ToilWDLStdLibBase(file_store, enforce_nonexistence=False)

		filename represents a URI or file name belonging to a WDL value of type value_type. work_dir represents
		the current working directory of the job and is where all relative paths will be interpreted from

Use sentinel value to deal with optional coerced files #4994

Use sentinel value to deal with optional coerced files #4994

Conversation

stxue1 commented Jun 28, 2024

Changelog Entry

Reviewer Checklist

Merger Checklist

adamnovak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stxue1 Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamnovak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stxue1 Jul 3, 2024 •

edited

Loading