fix: remote GS builds too many inventories; io:collect_mtime always uses uncached mtime

[cademirch]

Description

This PR includes two small changes that drastically reduce inventory building time when using Google remote files, as well as perhaps unnecessary extra mtime checks.

The IOCache class uses ExistsDicts to quickly lookup if a file is in the cache or not. Specifically, in

snakemake/snakemake/io.py

Lines 119 to 122 in 6f602a3

	def __contains__(self, path):
	# if already in inventory, always return True.
	parent = path.get_inventory_parent()
	return parent in self.has_inventory or super().__contains__(path)

ExistsDict checks if a parent of the query file has been inventoried.

This is problematic, because when we build the inventory for a remote GS file, we add bucket/subfolder to has_inventory

snakemake/snakemake/remote/GS.py

Line 195 in 6f602a3

cache.exists_remote.has_inventory.add("%s/%s" % (self.bucket_name, subfolder))

But only return self.bucket from get_inventory_parent

snakemake/snakemake/remote/GS.py

Lines 199 to 200 in 6f602a3

	def get_inventory_parent(self):
	return self.bucket_name

Which leads to many extra inventories being built. We fix this by returning self.bucket/subfolder.

The second commit in this PR I am less sure about. In IOCache , we build an mtime_inventory asynchronously . However, for reasons I'm not sure about, each async worker uses

snakemake/snakemake/io.py

Lines 174 to 175 in 6f602a3

	async def collect_mtime(self, path):
	return path.mtime_uncached

to get mtime info for a file. This is confusing to me as it explicitly uses mtime_uncached, which directly checks the mtime for a file, and for remote files this means expensive API calls. I'm not sure why mtime_uncached is used here, but it would be good to know if this is intentional.

Benchmarking these two changes shows some pretty impressive improvement.

Here is a simple workflow:

rule all:
    input: expand("results/{i}.txt", i=range(100))

rule make_file:
    output: "results/{i}.txt"
    shell: "echo {wildcards.i} > {output}"

Timed with GNU time three times.

Timing (no fix):

0:31.33 elapsed
0:35.06 elapsed
0:31.10 elapsed

Timing (with fix):

0:06.13 elapsed
0:06.12 elapsed
0:03.15 elapsed

The improvements are even more significant when testing with some of my larger workflows.

Would appreciate discussion/thoughts on this @johanneskoester and @vsoch and of course anyone else!

QC

• The PR contains a test case for the changes or the changes are already covered by an existing test case.
• 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).

[vsoch]

I would guess the timing difference comes down to using a cached value vs. requesting that one is not used. The second fix definitely looks like a bugfix (to include the dirname of the blob in the parent path). Did you test workflows without updating the mtime but with the second fix?

[cademirch]

I would guess the timing difference comes down to using a cached value vs. requesting that one is not used. The second fix definitely looks like a bugfix (to include the dirname of the blob in the parent path). Did you test workflows without updating the mtime but with the second fix?

I did but I didn't record it, I can do that now. I'm curious why we would directly use the uncached mtime when building the mtime inventory?

[vsoch]

I did but I didn't record it, I can do that now. I'm curious why we would directly use the uncached mtime when building the mtime inventory?

Mostly because a cached value may not be the most recent time it was modified. The question for snakemake is where / why would that matter? E.g., if we knew it was modified within the last day, is that enough, vs knowing it was an hour ago vs. three?

[cademirch]

I did but I didn't record it, I can do that now. I'm curious why we would directly use the uncached mtime when building the mtime inventory?

Mostly because a cached value may not be the most recent time it was modified. The question for snakemake is where / why would that matter? E.g., if we knew it was modified within the last day, is that enough, vs knowing it was an hour ago vs. three?

Hmm I guess I'm confused about how IOCache is used then. I thought it's built every execution of snakemake so it should be the most up to date information.

[vsoch]

Hmm I guess I'm confused about how IOCache is used then. I thought it's built every execution of snakemake so it should be the most up to date information.

Yeah - so we probably would not want a cached modified time, but I'll defer to @johanneskoester maybe that specificity doesn't matter as long as we can determine it was modified.

[cademirch]

Hmm I guess I'm confused about how IOCache is used then. I thought it's built every execution of snakemake so it should be the most up to date information.

Yeah - so we probably would not want a cached modified time, but I'll defer to @johanneskoester maybe that specificity doesn't matter as long as we can determine it was modified.

I'm not sure I agree. The iocache is created in workflow.py then passed to the DAG. Where it's used and updated by target files to check existence and mtime of needed files. So the cache is always up to date relative to the start of the workflow.

[cademirch]

@johanneskoester Any thoughts?

[cademirch]

@johanneskoester Thanks, applied your suggestions. Should be ready to merge.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
1.2% Duplication