Skip to content

Commit 3ffd8e1

Browse files
johanneskoestercoderabbitai[bot]
johanneskoester
and
coderabbitai[bot]
authored
fix: properly handle temp files in group jobs that are not needed outside of the group (do not upload them to storage and delete them early) (#3730)
### Description <!--Add a description of your PR here--> ### QC <!-- Make sure that you can tick the boxes below. --> * [x] The PR contains a test case for the changes or the changes are already covered by an existing test case. * [x] The documentation (`docs/`) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Temp-file cleanup is now group-aware and recursively handles grouped jobs, respecting group boundaries. * **Bug Fixes** * Prevents removal of temp files still needed by downstream jobs outside a group, avoiding downstream failures. * Improved diagnostic logging for temp-file decisions and group handling. * **Tests** * Added an end-to-end test, expected-result assets, and a helper script to validate group-aware temp-file behavior. * **Chores** * Spawned job invocations no longer force a notemp flag, restoring default temp-file behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 parent 6fb5821 commit 3ffd8e1

9 files changed

Lines changed: 108 additions & 13 deletions

File tree

‎.github/workflows/main.yml‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,14 @@ jobs:
5252
os: [ubuntu-latest, windows-2022, macos-latest] # , macos-13 not supported yet
5353
env: ["py311", "py312", "py313"]
5454
exclude:
55+
- os: windows-2022
56+
env: "py312"
5557
- os: windows-2022
5658
env: "py311"
5759
- os: macos-latest
5860
env: "py311"
61+
- os: macos-latest
62+
env: "py312"
5963

6064
runs-on: ${{ matrix.os }}
6165

‎src/snakemake/dag.py‎

Lines changed: 46 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -286,14 +286,19 @@ async def init(self, progress=False):
286286

287287
self.check_directory_outputs()
288288

289-
def get_unneeded_temp_files(self, job: AbstractJob) -> Iterable[str]:
289+
def get_unneeded_temp_files(self, job: Union[Job, GroupJob]) -> Iterable[str]:
290+
def get_files(job, group_job=None):
291+
for f in job.output:
292+
if is_flagged(f, "temp") and not self.is_needed_tempfile(
293+
job, f, outside_of_group_job=group_job
294+
):
295+
yield f
296+
290297
if isinstance(job, GroupJob):
291298
for j in job:
292-
yield from self.get_unneeded_temp_files(j)
299+
yield from get_files(j, group_job=job)
293300
else:
294-
for f in job.output:
295-
if is_flagged(f, "temp") and not self.is_needed_tempfile(job, f):
296-
yield f
301+
yield from get_files(job)
297302

298303
def check_directory_outputs(self):
299304
"""Check that no output file is contained in a directory output of the same or another rule."""
@@ -928,20 +933,49 @@ def temp_input(self, job: Union[Job, GroupJob]):
928933
if f in job_.temp_output and f not in skip:
929934
yield f
930935

931-
def is_needed_tempfile(self, job, tempfile):
932-
return (
933-
any(
934-
tempfile in files
935-
for j, files in self.depending[job].items()
936-
if not self.finished(j) and self.needrun(j) and j != job
936+
def is_needed_tempfile(self, job, tempfile, outside_of_group_job=None):
937+
"""Return whether a temp file is still needed by jobs other than the
938+
given and not part of the eventually given group.
939+
"""
940+
if self.workflow.subprocess_exec:
941+
return True
942+
943+
def is_other_group_or_no_group(j):
944+
return outside_of_group_job is None or j not in outside_of_group_job.jobs
945+
946+
assert self.workflow.storage_settings is not None
947+
948+
if self.workflow.remote_exec:
949+
is_unneeded_outside = (
950+
tempfile in self.workflow.storage_settings.unneeded_temp_files
937951
)
938-
or tempfile in self.derived_targetfiles
952+
else:
953+
is_unneeded_outside = True
954+
955+
is_derived_target = tempfile in self.derived_targetfiles
956+
is_needed_by_subsequent_job = any(
957+
tempfile in files
958+
for j, files in self.depending[job].items()
959+
if not self.finished(j)
960+
and self.needrun(j)
961+
and j != job
962+
and is_other_group_or_no_group(j)
963+
)
964+
logger.debug(
965+
f"Temp file {tempfile}: {is_unneeded_outside=}, {is_derived_target=}, "
966+
f"{is_needed_by_subsequent_job=}"
939967
)
968+
if is_unneeded_outside:
969+
return is_derived_target or is_needed_by_subsequent_job
970+
else:
971+
return True
940972

941973
async def handle_temp(self, job):
942974
"""Remove temp files if they are no longer needed. Update temp_mtimes."""
943975
if self.workflow.storage_settings.notemp:
976+
logger.debug("Not handling temp files since --notemp is set.")
944977
return
978+
logger.debug(f"Handle temp files for job {job}")
945979

946980
if job.is_group():
947981
for j in job:

‎src/snakemake/spawn_jobs.py‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -300,7 +300,6 @@ def general_args(
300300
),
301301
"--max-inventory-time 0",
302302
"--nocolor",
303-
"--notemp",
304303
"--no-hooks",
305304
"--nolock",
306305
"--ignore-incomplete",

‎tests/test_group_temp/Snakefile‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
rule all:
2+
input:
3+
"c.txt"
4+
output:
5+
"d.txt"
6+
shell:
7+
"echo d > {output}"
8+
9+
10+
rule a:
11+
output:
12+
temp("a.txt")
13+
group:
14+
"group1"
15+
shell:
16+
"echo a > {output}"
17+
18+
19+
rule b:
20+
input:
21+
"a.txt"
22+
output:
23+
"b.txt"
24+
group:
25+
"group1"
26+
shell:
27+
"echo b > {output}"
28+
29+
30+
rule c:
31+
input:
32+
"b.txt"
33+
output:
34+
"c.txt"
35+
group:
36+
"group1"
37+
shell:
38+
"""
39+
if [ -f a.txt ]; then
40+
echo "temp a.txt still exists!"
41+
exit 1
42+
fi
43+
echo c > {output}
44+
"""

‎tests/test_group_temp/expected-results/b.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
b

‎tests/test_group_temp/expected-results/c.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
c

‎tests/test_group_temp/expected-results/d.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
d

‎tests/test_group_temp/qsub‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
#!/bin/bash
2+
echo `date` >> qsub.log
3+
tail -n1 $1 >> qsub.log
4+
# simulate printing of job id by a random number
5+
echo $RANDOM
6+
sh $1

‎tests/tests.py‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2218,6 +2218,11 @@ def test_storage_cleanup_local():
22182218
assert not tmpdir_path.exists() or not any(tmpdir_path.iterdir())
22192219

22202220

2221+
@skip_on_windows
2222+
def test_group_temp():
2223+
run(dpath("test_group_temp"), cluster="./qsub")
2224+
2225+
22212226
@skip_on_windows # OS agnostic
22222227
def test_summary():
22232228
run(dpath("test01"), shellcmd="snakemake --summary", check_results=False)

0 commit comments

Comments
 (0)