[main] Parallelize per-sample test-pack generation in PipelineTestPackWriter - #387
Open
hugohills-regnosys wants to merge 2 commits into
Open
hugohills-regnosys wants to merge 2 commits into
hugohills-regnosys wants to merge 2 commits into
Conversation
writeTestPackSamples() generated every sample in a test pack sequentially on a single thread, even though each sample is fully independent (own input file, own output file, own SampleModel). For a model with tens of thousands of samples across many test packs, this loop dominates update-expectations runtime (e.g. one BNPP pipeline: ~17,700 samples at ~39ms each = ~11.4 minutes, single-threaded). Verified thread-safety of everything already shared across samples before parallelizing: RosettaTypeValidator, WorkflowPostProcessor and its constituent PostProcessSteps, and ReferenceResolverProcessStep all create their mutable state fresh per call and only hold immutable config/factory fields, so concurrent invocation is safe. The one real hazard was the single javax.xml.validation.Validator built once per test pack and reused across all its samples - Validator is explicitly not thread-safe. PipelineFunctionRunnerProvider/Impl now take a Schema instead and call schema.newValidator() per run() invocation (cheap, and Schema is safe to share). TransformTestExtension updated to match. Also synchronize the ValidationSummariser callback per sample, since arbitrary implementations of that interface aren't guaranteed thread-safe. writeTestPackSamples' per-sample body is extracted into generateSample() and driven by inputSamplesForTestPack.parallelStream(); output is re-sorted by sample ID afterward as before, so file layout and generated config are unchanged - only the generation order and wall-clock time.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PipelineTestPackWriter.writeTestPackSamples()generated every sample in a test pack sequentially on a single thread, even though each sample is fully independent (own input file, own output file, ownSampleModel). For models with large test-pack suites this loop dominatesupdate-expectationsruntime — observed on one large test-pack suite: tens of thousands of samples at tens of milliseconds each, adding up to double-digit minutes, single-threaded.inputSamplesForTestPack.parallelStream(), with the per-sample body extracted into a newgenerateSample()method. Output is still re-sorted by sample ID afterward, so file layout and generated config JSON are unchanged — only generation order and wall-clock time.Thread-safety review
Before parallelizing, I checked every piece of shared state samples pass through:
RosettaTypeValidator,WorkflowPostProcessorand its constituentPostProcessSteps (ReKeyProcessStep,GlobalKeyProcessStep,UpdateTemporaryKeyProcessStep,ReferenceResolverProcessStep,QualifyProcessorStep), and generated Rosetta functions all create their mutable state fresh per call and hold only immutable config/factory fields as instance state — safe for concurrent invocation.javax.xml.validation.Validatorwas built once per test pack and reused across all its samples.Validatoris explicitly documented as not thread-safe.PipelineFunctionRunnerProvider/PipelineFunctionRunnerProviderImpl/PipelineFunctionRunnerImplnow take/hold aSchemainstead and callschema.newValidator()fresh perrun()invocation (cheap, andSchemais safe to share across threads).TransformTestExtension(the only other consumer of this provider) is updated to match.ValidationSummariser.addValidationReport(...)is called per sample; since it's a caller-supplied interface with implementations outside this repo, the call is now synchronized rather than assumed thread-safe.Test plan
mvn test-compile— compiles cleanly.mvn test— all 94 tests pass, includingPipelineFunctionRunnerImplTest(updated to mockSchema.newValidator()instead of injecting aValidatordirectly),PipelineTestPackWriterTest,PipelineTestPackWriterDefaultSerialisationTest,TransformTestExtensionDefaultSerialisationTest.update-expectationsbuild to confirm wall-clock improvement and that generated expectation files are byte-for-byte unchanged.