AN-380 Make call cache hashing strategy configurable per filesystem and backend #7683

jgainerdewar · 2025-01-24T22:03:58Z

Description

See Configuring.md changes for a description of what this does and how it works.

Release Notes Confirmation

`CHANGELOG.md`

I updated CHANGELOG.md in this PR
I assert that this change shouldn't be included in CHANGELOG.md because it doesn't impact community users

Terra Release Notes

I added a suggested release notes entry in this Jira ticket
I assert that this change doesn't need Jira release notes because it doesn't impact Terra users

core/src/main/scala/cromwell/core/path/PathBuilder.scala

…nto jd_AN-380_hash

aednichols

Matches the mental picture I developed during mob times. Nice work!

aednichols · 2025-02-07T15:43:52Z

docs/Configuring.md

+###### Notes on GCS Hashing Strategy
+
+For some high-throughput production use cases that run many, many copies of the same task differing by only one input file, 
+the collision rate of `crc32c` may be unacceptably high. To dramatically reduce the chance of collision at the cost of 
+reducing the collection of tasks that can be call cached, we recommend `hashing-strategy: ["md5", "identity"]`. This 
+will use `md5` hashes when they exist, and fall back to the very strict `identity` strategy when they do not. Because
+all GCS files created by Cromwell are guaranteed to have `md5`, `identity` comes into play only for user-provided workflow
+input files.


Good explanation, one thing to add could be "why?" md5 does not exist, namely parallel composite upload

aednichols · 2025-02-07T18:44:54Z

filesystems/gcs/src/main/scala/cromwell/filesystems/gcs/batch/GcsBatchIoCommand.scala

@@ -174,23 +176,39 @@ object GcsBatchSizeCommand {
    file.objectBlobId.map(GcsBatchSizeCommand(file, _))
 }

-case class GcsBatchCrc32Command(override val file: GcsPath, override val blob: BlobId, setUserProject: Boolean = false)
-    extends IoHashCommand(file)
+case class GcsBatchHashCommand(override val file: GcsPath,


Extremely optional bikeshedding.

I've never been the biggest fan of this name, since this command is for a single file... it does not contain a batch.

Doubly so now that GCP Batch competes with GCS batch for headspace.

Suggested change

case class GcsBatchHashCommand(override val file: GcsPath,

case class GcsHashCommand(override val file: GcsPath,

I agree with you but would rather leave as-is and make a larger-scale change to this whole package later.

aednichols · 2025-02-07T19:02:15Z

engine/src/main/scala/cromwell/engine/io/nio/NioHashing.scala

-  private def getFileHashForGcsPath(gcsPath: GcsPath): IO[FileHash] = delayedIoFromTry {
-    gcsPath.objectBlobId.map(id => FileHash(HashType.GcsCrc32c, gcsPath.cloudStorage.get(id).getCrc32c))
-  }
+  private def getFileHashForGcsPath(gcsPath: GcsPath, hashStrategy: FileHashStrategy): IO[Option[FileHash]] =


Also extremely optional, I wonder if we could apply ✨ More Types ✨ to DRY out this group of functions.

Suggested change

private def getFileHashForGcsPath(gcsPath: GcsPath, hashStrategy: FileHashStrategy): IO[Option[FileHash]] =

private def getFileHashForPath[T](path: T, hashStrategy: FileHashStrategy[T]): IO[Option[FileHash]] =

We could, it would just mean that function would have a big match/case statement in it... which might be nicer, I'll give it a try. I kind of like the idea of doing cool stuff with FileHashStrategy[T] but don't want to hold this up that much further.

Discussed offline and decided to leave this as-is.

aednichols · 2025-02-07T19:07:30Z

backend/src/main/scala/cromwell/backend/standard/callcaching/StandardFileHashingActor.scala

+      fsConfigs <- configurationDescriptor.backendConfig.as[Option[Config]]("filesystems").toList
+      fsKey <- fsConfigs.root.keySet().asScala
+      configKey = s"${fsKey}.caching.hashing-strategy"
+      // TODO this allows users to override with an empty list to prevent caching, is that desirable or a footgun?


There is already a global call-caching.enabled flag that seems like a better choice.

I don't it makes sense to let users toggle call caching on a per-filesystem basis because one backend can use multiple filesystems in the same task (e.g. GCS + DRS).

I would make empty list an exception if call-caching.enabled is true.

Good point, will change.

LizBaldo

LGTM! I only have a few minor comments, the documentation is great and super clear to follow, especially for somebody not as familiar with call caching like me.

LizBaldo · 2025-02-11T21:47:06Z

backend/src/main/scala/cromwell/backend/standard/callcaching/StandardFileHashingActor.scala

+  val defaultHashingStrategies: Map[String, FileHashStrategy] = Map.empty
+
+  // Hashing strategy to use if none is specified.
+  val fallbackHashingStrategy: FileHashStrategy = FileHashStrategy.Md5


Should this be the list of md5 + identity?

This is the generic fallback across all backends and filesystems, since identity is only implemented for GCP I'm afraid that would confuse people.

That makes sense!

LizBaldo · 2025-02-11T22:45:43Z

core/src/main/scala/cromwell/core/callcaching/FileHashStrategy.scala

+}
+
+object FileHashStrategy extends LazyLogging {
+  // Define some commonly-used strategies so they're easier to refer to


Would it make sense to also add the identity strategy here?

We could, but I don't think it will ever be referred to in code since we aren't hard-coding it anywhere - we'll just be setting it in config. I'm a little reluctant to add technically dead code, but if you think it would be useful for documenting suggested strategies I can.

Oh gotcha, I am a bit fuzzy around the identity matching still, that is technically not a hash I guess, leaving it out is fine, especially if it would confuse the next dev working in that area.

docs/Configuring.md

LizBaldo · 2025-02-11T23:08:12Z

engine/src/test/scala/cromwell/engine/io/nio/NioFlowSpec.scala

-      }
-    }
-  }
+//  it should "fail if DrsPath hash doesn't match checksum" in {


Should this be removed?

Oh man good catch, I commented out these tests to "fix later" and then forgot about them. Later is now, will fix.

I resolved this by removing the functionality checked by these tests.

aednichols

Info: test failures are the requester-pays enablement scream test

filesystems/gcs/src/main/scala/cromwell/filesystems/gcs/batch/GcsBatchIoCommand.scala

jgainerdewar · 2025-02-12T21:27:32Z

filesystems/gcs/src/main/scala/cromwell/filesystems/gcs/GcsPathBuilder.scala

+  // blob id, which includes bucket name, blob name, and generation. This slash-delimited string is md5-hashed to
+  // obtain a string of a consistent length. This is used for call caching. Logic for generating this fingerprint
+  // is centralized here because different codepaths are dealing with different representations of the GS object.
+


Would love to DRY the below a little more, but not sure I can without finding the magical Google library type that StorageObject and Blob both inherit from. I've poked around a little for that and haven't found anything useful.

jgainerdewar added 8 commits January 24, 2025 14:28

Specify filesystemTypeKey on each Path subclass

e712005

Read per-filesystem AsyncFileHashingStrategy from config

449567c

Set default hashing strategies for each backend

4a36763

Move AsyncFileHashingStrategy to core

98beda3

Plumb hashStrategy through io commands, use for batch commands

7ea5618

Rename GcsBatchCrc32cCommand -> GcsBatchHashCommand

2f55cd4

Rename S3BatchTagCommand -> S3BatchHashCommand

6854497

Rename AsyncFileHashingStrategy -> FileHashStrategy

61eae83

jgainerdewar commented Jan 24, 2025

View reviewed changes

core/src/main/scala/cromwell/core/path/PathBuilder.scala Show resolved Hide resolved

jgainerdewar added 21 commits January 27, 2025 14:42

Update non-DRS NioHashing hash logic

be3a8a3

Progress on DRS

0c32295

Switch FileHashStrategy to list approach, make DRS conform

baf8151

Also check in this file

6b435d5

Eliminate special GcsCrc32c hash type

c10ae98

Lazily evaluate hashes

065ff05

Test fixes

df9dccc

Remove defunct tests

b82f2ec

Better handling for hex vs b64 crc32c representations

0a101df

Rename test file

add5a8c

Comments

de33a85

FileHashStrategy tests

875f9b5

Imports

16137f2

Tests

1966d28

Scalafmt

d0ad499

Remove a few TODOs

e30b7c2

Allow users to configure hash strategy as single string

05876b3

Explanatory comment for unprotected get

3a9e180

Cleanup

22cbc1f

Consistent blob id strings

83c381d

Ignore failing test

1458942

jgainerdewar added 2 commits February 4, 2025 16:44

Cleanup

c1c9c38

Merge branch 'develop' into jd_AN-380_hash

ed662f5

jgainerdewar marked this pull request as ready for review February 4, 2025 21:48

jgainerdewar requested a review from a team as a code owner February 4, 2025 21:48

jgainerdewar added 2 commits February 5, 2025 11:44

Docs

b3f04fd

Merge branch 'jd_AN-380_hash' of github.com:broadinstitute/cromwell i…

c03f985

…nto jd_AN-380_hash

aednichols approved these changes Feb 7, 2025

View reviewed changes

jgainerdewar added 3 commits February 11, 2025 15:08

PR Feedback

57ed4b4

Doc feedback

1f036c1

Fix test, clearer naming

cc0aba1

LizBaldo approved these changes Feb 11, 2025

View reviewed changes

aednichols approved these changes Feb 12, 2025

View reviewed changes

jgainerdewar commented Feb 12, 2025

View reviewed changes

filesystems/gcs/src/main/scala/cromwell/filesystems/gcs/batch/GcsBatchIoCommand.scala Outdated Show resolved Hide resolved

jgainerdewar added 3 commits February 12, 2025 15:55

Centralized (and correct!) logic for identity hashing

8c626c9

Fix typo

2071ecb

Remove unused checksum validation

46eb4bd

jgainerdewar commented Feb 12, 2025

View reviewed changes

aednichols and others added 2 commits February 14, 2025 14:31

AN-375 Metrics, logging for present vs. missing md5 (#7690)

892a5d4

Merge branch 'develop' into jd_AN-380_hash

c01fbb1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AN-380 Make call cache hashing strategy configurable per filesystem and backend #7683

AN-380 Make call cache hashing strategy configurable per filesystem and backend #7683

jgainerdewar commented Jan 24, 2025 •

edited

Loading

aednichols left a comment

aednichols Feb 7, 2025

aednichols Feb 7, 2025

jgainerdewar Feb 11, 2025

aednichols Feb 7, 2025

jgainerdewar Feb 11, 2025

jgainerdewar Feb 11, 2025

aednichols Feb 7, 2025

jgainerdewar Feb 11, 2025

LizBaldo left a comment

LizBaldo Feb 11, 2025

jgainerdewar Feb 12, 2025

LizBaldo Feb 12, 2025

LizBaldo Feb 11, 2025

jgainerdewar Feb 12, 2025

LizBaldo Feb 12, 2025

LizBaldo Feb 11, 2025

jgainerdewar Feb 12, 2025

jgainerdewar Feb 13, 2025

aednichols left a comment

jgainerdewar Feb 12, 2025

	case class GcsBatchHashCommand(override val file: GcsPath,
	case class GcsHashCommand(override val file: GcsPath,

	private def getFileHashForGcsPath(gcsPath: GcsPath, hashStrategy: FileHashStrategy): IO[Option[FileHash]] =
	private def getFileHashForPath[T](path: T, hashStrategy: FileHashStrategy[T]): IO[Option[FileHash]] =

AN-380 Make call cache hashing strategy configurable per filesystem and backend #7683

Are you sure you want to change the base?

AN-380 Make call cache hashing strategy configurable per filesystem and backend #7683

Conversation

jgainerdewar commented Jan 24, 2025 • edited Loading

Description

Release Notes Confirmation

CHANGELOG.md

Terra Release Notes

aednichols 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

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

LizBaldo 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

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

aednichols left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgainerdewar commented Jan 24, 2025 •

edited

Loading

`CHANGELOG.md`