Allow CWL workflows to have jobs use all of a Slurm node's memory #5052

adamnovak · 2024-08-08T18:05:17Z

This should fix #4971. Instead of --defaultMemory 0, to run CWL jobs that lack their own ramMin with a full Slurm node's memory, you would now pass --no-cwl-default-ram --slurmDefaultAllMem=True.

This might cause some new problems:

Other internal CWL runner jobs that expected to use the default memory would now use all the memory on their node, if submitted to the cluster.
Now we use the CWL spec's required default memory for jobs that don't specify a limit, instead of --defaultMemory unless the user passes --no-cwl-default-ram. Previously I think we were ignoring the spec and always using the Toil --defaultMemory. This might break some workflow runs that used to work because of us giving them more memory than the spec said to.

Also, #4971 says we're supposed to implement a real framework for doing this kind of memory expansion across all batch systems that support it. But I didn't want to add a new bool flag onto Requirer for such a specific purpose. Probably if we need it we should combine it with preemptible somehow into a tag/flag system. Or we could implement memory range requirements and allow the top of the range to be unbounded, or treat some threshold upper limit as "all the node's memory" in the Slurm batch system.

Changelog Entry

To be copied to the draft changelog by merger:

Toil now has a --slurmDefaultAllMem option to run jobs lacking their own memory requirements with Slurm's --mem=0, so they get a whole node's memory.
toil-cwl-runner now has --no-cwl-default-ram (and --cwl-default-ram) to control whether the CWL spec's default ramMin is applied, or Toil's own default memory logic is used.
The --dont_allocate_mem and --allocate_mem options have been deprecated and replaced with --slurmAllocateMem, which can be True or False.

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.

…options and env vars

…ptions

…ments use whole nodes' memory

adamnovak · 2024-08-08T18:05:55Z

I still need to manually test this to make sure it actually does what it is meant to do.

…cleanup on kill

adamnovak · 2024-08-08T20:45:56Z

I wrote a test for this and it does indeed seem to issue jobs that ask for whole Slurm nodes when I use the two new options together.

I also fixed Slurm job cleanup when a workflow is killed; it wasn't doing that before because shutdown() wasn't doing any killing in AbstractGridEngineBatchSystem. I needed this for the test to not leave behind pending jobs when there aren't any free entire cluster nodes.

adamnovak · 2024-09-10T17:27:14Z

@DailyDreaming Can you review this?

DailyDreaming

LGTM. Some minor clean-up.

DailyDreaming · 2024-09-30T22:03:08Z

docs/appendices/environment_vars.rst

@@ -131,6 +131,9 @@ There are several environment variables that affect the way Toil runs.
 | TOIL_GOOGLE_PROJECTID            | The Google project ID to use when generating       |
 |                                  | Google job store names for tests or CWL workflows. |
 +----------------------------------+----------------------------------------------------+
+| TOIL_SLURM_ALLOCATE_MEM          | Whether to akllocate memory in Slurm with --mem.   |


DailyDreaming · 2024-09-30T22:05:04Z

docs/appendices/environment_vars.rst

@@ -143,6 +146,8 @@ There are several environment variables that affect the way Toil runs.
 |                                  | jobs.                                              |
 |                                  | There is no default value for this variable.       |
 +----------------------------------+----------------------------------------------------+
+| TOIL_SLURM_TIME                  | Slurm job time limit, in [DD-]HH:MM:SS format.     |


An example might be useful (literally adding e.g. TOIL_SLURM_TIME="04:00:00" for 4 hours, assuming that this produces a 4 hour time limit), or a link to SLURM docs if this is a direct alias for a slurm arg.

DailyDreaming · 2024-09-30T22:07:58Z

docs/cwl/running.rst

+
+Besides the normal Toil options and the options supported by cwltool, toil-cwl-runner adds some of its own options:
+
+  --bypass-file-store   Do not use Toil's file store and assume all paths are accessible in place from all nodes.


Maybe a further explanation that files may not be redundantly copied to a potentially remote filestore, and this can speed up the workflow (as a reason for why someone might want this). Has the downside that the workflow is not restart-able from the filestore if it made progress and failed part-way through.

DailyDreaming · 2024-09-30T22:09:40Z

docs/running/cliOptions.rst

+  --slurmTime SLURM_TIME
+                        Slurm job time limit, in [DD-]HH:MM:SS format.
+  --slurmPE SLURM_PE    Special partition to send Slurm jobs to if they ask
+                        for more than 1 CPU.


Could this be explained further?

DailyDreaming · 2024-09-30T22:12:02Z

src/toil/batchSystems/abstractGridEngineBatchSystem.py

@@ -417,11 +433,11 @@ def issueBatchJob(self, command: str, jobDesc, job_environment: Optional[Dict[st
            else:
                gpus = jobDesc.accelerators


This code looks like it was maybe left in and is a function now (i.e. self.count_needed_gpus)?

DailyDreaming · 2024-09-30T22:17:52Z

src/toil/options/cwl.py

+        "--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."


Space needed at the end of this sentence.

DailyDreaming · 2024-09-30T22:18:10Z

src/toil/options/cwl.py

+    )
+    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,"


Space needed at the end of this sentence.

DailyDreaming · 2024-09-30T22:20:02Z

src/toil/test/cwl/cwlTest.py

+            # Reap it
+            child.wait()
+            # The test passes
+            return


I think that this return is a no-op?

I think I put it there to make it clear that we intend to go there and end the test. But if you think it makes more sense without the return I'll drop it.

…-node-memory

adamnovak added 6 commits August 8, 2024 11:53

Remove periods form option section headings

0fd0c52

Hide entire service section from CWL help

b0c395f

Change Slurm memory allocation flag to new style, and document Slurm …

aa1d275

…options and env vars

Use but add a way to bypass the CWL spec default RAM, and rearrange o…

7fc1f0c

…ptions

Add --slurmDefaultAllMem to let jobs without their own memory require…

1a397b3

…ments use whole nodes' memory

Actually set new config entry

768f227

adamnovak changed the title ~~Issues/4971 slurm node memory~~ Allow CWL workflows to have jobs use all of a Slurm node's memory Aug 8, 2024

adamnovak added 2 commits August 8, 2024 15:33

Add a test for whole-node CWL memory on Slurm

ea1e3ea

Detect a job not getting a node as a passing test, and fix Slurm job …

4b06802

…cleanup on kill

adamnovak marked this pull request as ready for review August 8, 2024 20:46

github-actions bot and others added 2 commits August 14, 2024 18:35

Merge master into issues/4971-slurm-node-memory

632c3b7

Fix Slurm issue method typing

5d04bd1

adamnovak requested a review from DailyDreaming August 14, 2024 18:41

github-actions bot added 12 commits August 15, 2024 14:47

Merge master into issues/4971-slurm-node-memory

ea52789

Merge master into issues/4971-slurm-node-memory

a6b2f75

Merge master into issues/4971-slurm-node-memory

2a6c3b1

Merge master into issues/4971-slurm-node-memory

104cf3c

Merge master into issues/4971-slurm-node-memory

2e71b7c

Merge master into issues/4971-slurm-node-memory

4960555

Merge master into issues/4971-slurm-node-memory

da79265

Merge master into issues/4971-slurm-node-memory

613df48

Merge master into issues/4971-slurm-node-memory

b8f410b

Merge master into issues/4971-slurm-node-memory

77dcc47

Merge master into issues/4971-slurm-node-memory

4ed2a07

Merge master into issues/4971-slurm-node-memory

ac5b298

github-actions bot added 2 commits September 12, 2024 23:05

Merge master into issues/4971-slurm-node-memory

97e98ce

Merge master into issues/4971-slurm-node-memory

9939309

adamnovak and others added 3 commits September 24, 2024 14:29

Merge branch 'master' into issues/4971-slurm-node-memory

93c2dbf

Merge branch 'master' into issues/4971-slurm-node-memory

c132323

Merge master into issues/4971-slurm-node-memory

e6f42cf

DailyDreaming approved these changes Sep 30, 2024

View reviewed changes

github-actions bot and others added 12 commits October 1, 2024 23:35

Merge master into issues/4971-slurm-node-memory

286ea86

Merge master into issues/4971-slurm-node-memory

bbdd778

Merge master into issues/4971-slurm-node-memory

301566f

Merge master into issues/4971-slurm-node-memory

9cdd365

Merge master into issues/4971-slurm-node-memory

0034548

Merge master into issues/4971-slurm-node-memory

8d09bd3

Merge master into issues/4971-slurm-node-memory

b376f54

Fix docs and remove dead code from code review

6f4c864

Merge master into issues/4971-slurm-node-memory

488f965

Merge master into issues/4971-slurm-node-memory

a634866

Merge remote-tracking branch 'upstream/master' into issues/4971-slurm…

f59b898

…-node-memory

Drop duplicate import options

813efe6

adamnovak merged commit 631ae0c into master Oct 14, 2024
3 checks passed

adamnovak deleted the issues/4971-slurm-node-memory branch October 14, 2024 19:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow CWL workflows to have jobs use all of a Slurm node's memory #5052

Allow CWL workflows to have jobs use all of a Slurm node's memory #5052

adamnovak commented Aug 8, 2024

adamnovak commented Aug 8, 2024

adamnovak commented Aug 8, 2024

adamnovak commented Sep 10, 2024

DailyDreaming left a comment

DailyDreaming Sep 30, 2024

DailyDreaming Sep 30, 2024

DailyDreaming Sep 30, 2024

DailyDreaming Sep 30, 2024

DailyDreaming Sep 30, 2024 •

edited

Loading

DailyDreaming Sep 30, 2024

DailyDreaming Sep 30, 2024

DailyDreaming Sep 30, 2024

adamnovak Oct 7, 2024


		Besides the normal Toil options and the options supported by cwltool, toil-cwl-runner adds some of its own options:

		--bypass-file-store Do not use Toil's file store and assume all paths are accessible in place from all nodes.

		@@ -417,11 +433,11 @@ def issueBatchJob(self, command: str, jobDesc, job_environment: Optional[Dict[st
		else:
		gpus = jobDesc.accelerators

Allow CWL workflows to have jobs use all of a Slurm node's memory #5052

Allow CWL workflows to have jobs use all of a Slurm node's memory #5052

Conversation

adamnovak commented Aug 8, 2024

Changelog Entry

Reviewer Checklist

Merger Checklist

adamnovak commented Aug 8, 2024

adamnovak commented Aug 8, 2024

adamnovak commented Sep 10, 2024

DailyDreaming left a 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

DailyDreaming Sep 30, 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

DailyDreaming Sep 30, 2024 •

edited

Loading