Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 56 commits into from
Oct 1, 2024

Conversation

stxue1
Copy link
Contributor

@stxue1 stxue1 commented Jul 19, 2024

This should solve #5004

This also reverts #4994 and introduces another fix for #4988; there should be behavioral parity with miniwdl

This should also resolve #5031

This is a bit of an overhaul of how we handle files in toil-wdl-runner; instead of immediately virtualizing upon seeing a File type (coerced or not), the WDL value is kept as a path until the last second. Only before tasks are sent off, the files will be virtualized. This way, the WDL side functionality should still be the same while allowing for File to String coercion as we no longer replace all File paths with our virtualized representation immediately.

Monkeypatching is removed as we no longer virtualize at the coercion step, but before/after function calls and manually at task boundaries.

I don't think this will increase the amount of IO. One edge case is if the user changes the filename right before being put into a function; toil will virtualize the new filename for that function call and keep both the original and new virtualized file around in the jobstore until the job is completed. It may be worth removing the new virtualized file from the jobstore after use, but I'm not sure where to hook in the behavior.

The downside is that all jobs that are marked as local must always be local in the future. I explicitly set the flag in the class constructors where I could. I think the virtualization behavior should ensure local jobs on workers should work.

The function evaluate_bindings_from_decls is now used for all decl parsing/evaluation to reduce duplicative code.

Changelog Entry

To be copied to the draft changelog by merger:

  • File virtualization in toil-wdl-runner now only happens at task boundaries
    • File to String coercion should be supported

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.

…luation. Also gets rid of monkeypatching in favor of a manual function call
src/toil/wdl/wdltoil.py Outdated Show resolved Hide resolved
@stxue1 stxue1 marked this pull request as draft July 23, 2024 03:17
stxue1 added 2 commits July 24, 2024 12:50
…alid coerced-to-null files and raise if exception found
@stxue1 stxue1 marked this pull request as ready for review July 25, 2024 01:14
@stxue1 stxue1 linked an issue Jul 25, 2024 that may be closed by this pull request
src/toil/wdl/wdltoil.py Outdated Show resolved Hide resolved
Copy link
Member

@adamnovak adamnovak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned that by leaving relative URLs from the input JSON as relative URLs in WDL Files and then trying to figure out where they were meant to be relative to later, we've introduced a whole universe of cross-talk that we don't want to deal with. Maybe we can poll for where they ought to come from and turn them into absolute URLs before handing them to the workflow?

I like that we're ripping out the coercion monkey-patch and also nonexistent:, which simplifies things. But are we sure that we retain support for using Toil's supported URI schemes when MiniWDL reads a file? File the_file = "gs://google-bucket-name/filename.txt" ought to work if Toil has Google Storage support installed, even if MiniWDL doesn't know about reading Google Storage URLs. I think the coercion hook was doing that for us and I can't really tell if something is taking its place. If we're losing that feature and we think it is worth it, we should know we're losing it.

I'd also like to see a new comment at the top describing how the system works now, and what invariants or abstractions it imposes that new code is going to need to follow to keep it working. File holds a URL or leader-local path at the workflow level. When we actually read it with read_lines() at the workflow level, does MiniWDL fetch it? If we read_lines() it twice, do we have a cache? If we load it into the file store when it enters a task, and it is passed out of the task unmodified, do we see the original URL in the task output (because everything at workflow level is always an original URL) or the imported file store one?

Is talking about "virtualized" (visible to WDL code) and "devirtualized" (visible to Python open()) file names still the right way to understand what the code is doing? Or should we be using different terms or concepts to think about the packing and unpacking that happens when taking Bindings in and out of tasks?

I also like the idea of wdl_options to hold the global settings for the run, kind of like the CWL RuntimeContext. But I think it needs a type so we can have a better handle on what's allowed to be in there.

src/toil/wdl/wdltoil.py Outdated Show resolved Hide resolved
src/toil/wdl/wdltoil.py Outdated Show resolved Hide resolved
src/toil/wdl/wdltoil.py Outdated Show resolved Hide resolved
src/toil/wdl/wdltoil.py Outdated Show resolved Hide resolved
src/toil/wdl/wdltoil.py Outdated Show resolved Hide resolved
src/toil/wdl/wdltoil.py Outdated Show resolved Hide resolved
src/toil/wdl/wdltoil.py Outdated Show resolved Hide resolved
src/toil/wdl/wdltoil.py Show resolved Hide resolved
src/toil/wdl/wdltoil.py Outdated Show resolved Hide resolved
Comment on lines 1605 to 1625
if isinstance(value, WDL.Value.File):
pass
elif isinstance(value, WDL.Value.Array) and isinstance(expected_type, WDL.Type.Array):
for elem, orig_elem in zip(value.value, original_value.value):
map_over_files_in_value_check_null_type(elem, orig_elem, expected_type.item_type)
elif isinstance(value, WDL.Value.Map) and isinstance(expected_type, WDL.Type.Map):
for pair, orig_pair in zip(value.value, original_value.value):
# The key of the map cannot be optional or else it is not serializable, so we only need to check the value
map_over_files_in_value_check_null_type(pair[1], orig_pair[1], expected_type.item_type[1])
elif isinstance(value, WDL.Value.Pair) and isinstance(expected_type, WDL.Type.Pair):
map_over_files_in_value_check_null_type(value.value[0], original_value.value[0], expected_type.left_type)
map_over_files_in_value_check_null_type(value.value[1], original_value.value[1], expected_type.right_type)
elif isinstance(value, WDL.Value.Struct) and isinstance(expected_type, WDL.Type.StructInstance):
for (k, v), (_, orig_v) in zip(value.value.items(), original_value.value.items()):
# The parameters method for WDL.Type.StructInstance returns the values rather than the dictionary
# While dictionaries are ordered, this should be more robust; the else branch should never be hit
if expected_type.members is not None:
map_over_files_in_value_check_null_type(v, orig_v, expected_type.members[k])
elif isinstance(value, WDL.Value.Null):
if not expected_type.optional:
raise FileNotFoundError(errno.ENOENT, os.strerror(errno.ENOENT), original_value.value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having one version of this traversal logic is what the map function was supposed to be for. But I guess here we're traversing two mirrored structures in parallel so we can't use the other code.

@adamnovak
Copy link
Member

adamnovak commented Aug 1, 2024

@stxue1 Another thing I thought of is, if we need to make local=True always run a job on the leader and never run the job elsewhere, as part of this, then we need to remove the command lien option that lets you run local jobs on workers. Otherwise using it will break WDL workflows.

We also might need to revise the batch system documentation, and maybe the inheritance hierarchy, since you would need all batch systems, even those coming from plugins, to always have the local job logic.

@adamnovak
Copy link
Member

We think this will also fix #4992.

@adamnovak
Copy link
Member

@stxue1 If this fixes #5031, it also needs to enable the test from the conformance tests; it looks like this doesn't touch the tests file.

Copy link
Member

@adamnovak adamnovak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still a couple things I would change, but I think this is fine.

Comment on lines 101 to 106
# WDL options to pass into the WDL jobs and standard libraries
# task_path: Dotted WDL name of the part of the workflow this library is working for.
# execution_dir: Directory to use as the working directory for workflow code.
# container: The type of container to use when executing a WDL task. Carries through the value of the commandline --container option
WDL_Context = TypedDict('WDL_Context', {"execution_dir": NotRequired[str], "container": NotRequired[str],
"task_path": str, "namespace": str})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have docs for namespace.

src/toil/wdl/wdltoil.py Outdated Show resolved Hide resolved
src/toil/wdl/wdltoil.py Outdated Show resolved Hide resolved
# task_path: Dotted WDL name of the part of the workflow this library is working for.
# execution_dir: Directory to use as the working directory for workflow code.
# container: The type of container to use when executing a WDL task. Carries through the value of the commandline --container option
WDL_Context = TypedDict('WDL_Context', {"execution_dir": NotRequired[str], "container": NotRequired[str],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't use an _ in the name here; it's a different convention than all the other types.

@adamnovak
Copy link
Member

@stxue1 It looks like conformance test 27 ("Legacy test for type_pair_with_files") is failing because it is trying to devirtualize a file to the same path twice. I think it can't deal with how that test sends the same path twice in the input JSON, so there are two File values from the same directory with the same basename (because it's the same file twice).

@stxue1 stxue1 requested a review from adamnovak September 27, 2024 20:21
@stxue1
Copy link
Contributor Author

stxue1 commented Sep 28, 2024

I changed the function convert_remote_files, so that function needs a rereview

Copy link
Member

@adamnovak adamnovak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the changes are probably fine, but could be slightly better.

:param import_remote_files: If set, import files from remote locations. Else leave them as URI references.
"""
path_to_id: Dict[str, uuid.UUID] = {}
@memoize
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this memoization gets scoped to the call to the enclosing function, but I also think that's what we want here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's what we want, it is the same way we did memoization in the previous implementation. I believe file imports happening within the WDL have their own caching system separate from this anyway.

candidate_uri, toil_uri = import_filename(file.value)
if candidate_uri is None and toil_uri is None:
# If we get here we tried all the candidates
raise RuntimeError(f"Could not find {file.value} at any of: {list(potential_absolute_uris(file.value, search_paths if search_paths is not None else []))}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's odd to re-do the calculation of the possible locations here. If it changes it needs to change in both places. Having a function to compute it helps with that, but then we have a long expression going into the function and it's hard to tell if it is in sync just by looking.

Maybe the list of tried locations really wants to be an output from import_filename? Or maybe import_filename could raise this, since it is only called in the one place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to have it inside import_filename, though the function wouldn't ever have a base return case, and I'm not sure if that's better or worse behaviorally. My thinking was that even though it is called twice, it will only be called again if a runtime error is hit, which is unlikely.

@stxue1 stxue1 merged commit 0034c92 into master Oct 1, 2024
1 check passed
@stxue1 stxue1 deleted the issues/5004-wdl-virtualize-only-at-task-boundaries branch October 1, 2024 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants