Support importing on workers in WDL #5103

stxue1 · 2024-09-26T00:06:05Z

Mirrors #5025 to the WDL side too

Changelog Entry

To be copied to the draft changelog by merger:

Added support to import files on workers for 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 · 2024-09-26T16:32:35Z

Don't you mean toil-wdl-runner in the changelog entry?

adamnovak

I would like to see at least the command line options shared between WDL and CWL here, although WDL might want to have the --kebab-case forms conditioned out.

What would we need to do to make the same importer job class able to be used for both CWL and WDL? If we made it a job that takes a list of file paths/URIs and some search path information, and returns a list of found absolute paths/URIs and Toil file IDs, would we then be able to write the needed adapters?

adamnovak · 2024-09-26T16:34:13Z

src/toil/options/wdl.py

+    parser.add_argument(
+        "--runImportsOnWorkers", "--run-imports-on-workers",
+        action="store_true", default=False,
+        help=suppress_help or "Run the file imports on a worker instead of the leader. This is useful if the leader is not optimized for high network performance."
+                              "If set to true, the argument --importWorkersDisk must also be set.",
+        dest="run_imports_on_workers"
+    )
+
+    parser.add_argument("--importWorkersDisk", "--import-workers-disk",
+                        help=suppress_help or "Specify the amount of disk space an import worker will use. If file streaming for input files is not available,"
+                                              "this should be set to the size of the largest input file. This must be set in conjunction with the argument runImportsOnWorkers.",
+                        dest="import_workers_disk",
+                        type=lambda x: human2bytes(str(x)),
+                        default=None)


I don't think any of the other WDL-side options have the --kebab-case form.

Also, is it possible to share the option definitions for these between CWL and WDL?

If the function signatures are different, I think it's easier to keep them separate than figuring out a way to get around argparse, unless we remove kebab case from the CWL equivalent of these options, which I originally included as all other CWL options are kebab case

I think I can actually move it out into a separate function

src/toil/wdl/wdltoil.py

adamnovak · 2024-09-26T16:38:01Z

src/toil/wdl/wdltoil.py

+    # Take care of incompatible arguments related to file imports
+    if options.run_imports_on_workers is True and options.import_workers_disk is None:
+        logger.error("Commandline arguments --runImportsOnWorkers and --importWorkersDisk must both be set to run file imports on workers.")
+        return


We probably need to raise an error or exit with a nonzero exit code, or the workflow will look like it succeeded.

Changed to a RuntimeError

adamnovak · 2024-09-26T16:38:58Z

src/toil/wdl/wdltoil.py

+        # Import any files in the bindings
+        input_bindings = import_files(inputs, target.name, toil._jobStore, inputs_search_path, skip_remote=options.reference_inputs)
+        # Run the workflow and get its outputs namespaced with the workflow name.
+        root_job = WDLRootJob(target, input_bindings, wdl_options=wdl_options)


We probably need to rename WDLRootJob since it isn't always the root anymore.

Renamed to WDLStartJob

Co-authored-by: Adam Novak <anovak@soe.ucsc.edu>

…25-import-on-workers-wdl

…incompatible options are detected

adamnovak · 2024-09-30T22:00:15Z

@stxue1 I think you forgot to add src/toil/options/runner.py to Git.

stxue1 · 2024-09-30T22:02:32Z

@stxue1 I think you forgot to add src/toil/options/runner.py to Git.

Whoops, added

…osphere/toil into issues/5025-import-on-workers-wdl

…25-import-on-workers-wdl

adamnovak

This looks like it should work.

adamnovak · 2024-10-07T15:48:00Z

src/toil/options/runner.py

+    run_imports_on_workers_arguments = ["--runImportsOnWorkers"]
+    if cwl:
+        run_imports_on_workers_arguments.append("--run-imports-on-workers")
+    parser.add_argument(*run_imports_on_workers_arguments, action="store_true", default=False, dest="run_imports_on_workers",


I think we usually like to make new boolean options use str2bool and accept False/True, since we've been upgrading so many of the old ones to that format. Maybe we should upgrade this one too?

stxue1 and others added 2 commits September 25, 2024 17:04

mirror import on workers update to wdl

33fa295

Merge master into issues/5025-import-on-workers-wdl

9a6612a

adamnovak requested changes Sep 26, 2024

View reviewed changes

github-actions bot and others added 6 commits September 26, 2024 16:43

Merge master into issues/5025-import-on-workers-wdl

e90bfbe

Apply suggestions from code review

a46a001

Co-authored-by: Adam Novak <anovak@soe.ucsc.edu>

Merge branch 'master' of github.com:DataBiosphere/toil into issues/50…

c55330c

…25-import-on-workers-wdl

Rename WDLRootJob, remove kebab case from WDL, and raise an error if …

413e3ea

…incompatible options are detected

Move out shared runner options

0fcdf35

Merge master into issues/5025-import-on-workers-wdl

d615a4e

Add runner.py

361551d

Merge branch 'issues/5025-import-on-workers-wdl' of github.com:DataBi…

10f02f2

…osphere/toil into issues/5025-import-on-workers-wdl

adamnovak mentioned this pull request Oct 2, 2024

Add import on workers for WDL #5113

Closed

Merge branch 'master' of github.com:DataBiosphere/toil into issues/50…

c6b4079

…25-import-on-workers-wdl

stxue1 linked an issue Oct 3, 2024 that may be closed by this pull request

Add import on workers for WDL #5113

Closed

stxue1 and others added 8 commits October 3, 2024 12:04

Add logDebug to giraffe workflow

9329327

Add toil.import_file logic into the wdl import function

881ab9a

Merge master into issues/5025-import-on-workers-wdl

1bc9b3e

Merge master into issues/5025-import-on-workers-wdl

b6f2ab0

Merge master into issues/5025-import-on-workers-wdl

b016723

Merge master into issues/5025-import-on-workers-wdl

f626dea

Merge master into issues/5025-import-on-workers-wdl

a1ebbf4

Merge master into issues/5025-import-on-workers-wdl

7b2e167

adamnovak approved these changes Oct 7, 2024

View reviewed changes

adamnovak and others added 3 commits October 7, 2024 11:54

Reconnect test command argument to its option

3ba0391

Merge master into issues/5025-import-on-workers-wdl

0a17f9c

Merge master into issues/5025-import-on-workers-wdl

3f8c8ca

stxue1 merged commit 8f665a9 into master Oct 9, 2024
1 check passed

stxue1 deleted the issues/5025-import-on-workers-wdl branch October 9, 2024 19:22

stxue1 mentioned this pull request Oct 24, 2024

Add filesize sniffing and parallelize importing on jobs #5133

Open

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support importing on workers in WDL #5103

Support importing on workers in WDL #5103

stxue1 commented Sep 26, 2024 •

edited

Loading

adamnovak commented Sep 26, 2024

adamnovak left a comment •

edited

Loading

adamnovak Sep 26, 2024

stxue1 Sep 27, 2024

stxue1 Sep 28, 2024

adamnovak Sep 26, 2024

stxue1 Sep 27, 2024

adamnovak Sep 26, 2024

stxue1 Sep 27, 2024

adamnovak commented Sep 30, 2024

stxue1 commented Sep 30, 2024

adamnovak left a comment

adamnovak Oct 7, 2024

Support importing on workers in WDL #5103

Support importing on workers in WDL #5103

Conversation

stxue1 commented Sep 26, 2024 • edited Loading

Changelog Entry

Reviewer Checklist

Merger Checklist

adamnovak commented Sep 26, 2024

adamnovak left a comment • 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

adamnovak commented Sep 30, 2024

stxue1 commented Sep 30, 2024

adamnovak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stxue1 commented Sep 26, 2024 •

edited

Loading

adamnovak left a comment •

edited

Loading