Skip to content

SeedService: give unnamed and named engines of one module distinct seeds - #1985

Open
oksuzian wants to merge 1 commit into
Mu2e:mainfrom
oksuzian:fix/seedservice-engineid-order
Open

oksuzian wants to merge 1 commit into
Mu2e:mainfrom
oksuzian:fix/seedservice-engineid-order

Conversation

@oksuzian

Copy link
Copy Markdown
Collaborator

What

SeedServiceHelper::EngineId now compares (moduleLabel, instanceDefined, instanceName) as a tuple in both operator< and operator==.

Why

EngineId::operator< compared instance names only when both ids had one, so an id without an instance name (g4) was an equivalent key to every id of the same module with one (g4.x, g4.y) in knownSeeds_. SeedService::getSeed returns the cached seed when the insert finds an equivalent key, before ensureUnique runs. As a result:

  • getSeed() followed by getSeed("x") in one module returned the same seed for both engines. The header promises an exception when two engines share a seed.
  • Mixing the two forms broke the strict weak ordering std::map requires (g4 ~ g4.x and g4 ~ g4.y, but g4.x < g4.y), so which entry got reused depended on call order.

A standalone program reproduces this, replaying getSeed's insert-or-return logic against the real header. Seeds are shown as base + offset, with base 100:

calls before after
getSeed(), getSeed("x") 100, 100 100, 101
getSeed("x"), getSeed("y"), getSeed() 100, 101, 101 100, 101, 102
getSeed("y"), getSeed(), getSeed("x") 100, 100, 101 100, 101, 102
gen, g4, mix.a, mix.b, gen (one form per module) 100, 101, 102, 103, 100 identical

Impact on existing jobs

None expected. Seeds are assigned in call order (currentSeed_++), not map order, and no Offline module other than SeedTest01 passes an instance name. Every module that uses a single form gets exactly the seeds it got before; only the mixed case changes.

Validation

  • g++ -std=c++20 -Wall -Wextra -Werror -fsyntax-only on SeedService.cc, SeedService_service.cc and SeedTest01_module.cc in the muse al9 prof environment: clean.
  • The before/after comparison above.
  • Not run: the SeedService/test/*.fcl jobs. None of them mixes the two forms within one module.

Deliberately not in this PR

These came up in the same review of SeedService:

  • ensureRange checks only the upper bound, so a negative preDefinedOffset is accepted, and baseSeed > 0 is never validated. This will be a separate PR.
  • A question for the service owners: automaticSeeds uses autoIncrement while the job tools set baseSeed = 1 + index. Seeds are unique within a job, but neighbouring jobs share seed values between different modules. linearMapping would keep the job ranges disjoint.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PgCD6rzgTteUFTPsdvWcou

EngineId::operator< compared instance names only when both ids had one,
so "g4" and "g4.x" were equivalent keys in knownSeeds_. A module calling
getSeed() and getSeed("x") received the same seed for both engines, and
getSeed returned the cached value before ensureUnique could throw. Mixing
the two forms also broke the strict weak ordering std::map requires, so
which entry was reused depended on call order.

Compare (moduleLabel, instanceDefined, instanceName) as a tuple in both
operator< and operator==. Seeds are still assigned in call order, so a
module using only one form gets exactly the seeds it got before.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PgCD6rzgTteUFTPsdvWcou
@FNALbuild

Copy link
Copy Markdown
Collaborator

Hi @oksuzian,
You have proposed changes to files in these packages:

  • SeedService

which require these tests: build.

@Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main.

⌛ The following tests have been triggered for e304eab: build (Build queue - API unavailable)

About FNALbuild. Code review on Mu2e/Offline.

@FNALbuild

Copy link
Copy Markdown
Collaborator

☀️ The build tests passed at e304eab.

Test Result Details
test with Command did not list any other PRs to include
merge Merged e304eab at 329c100
build (prof) Log file. Build time: 04 min 23 sec
ceSimReco Log file.
g4test_03MT Log file.
transportOnly Log file.
POT Log file.
g4study Log file.
cosmicSimReco Log file.
cosmicOffSpill Log file.
ceSteps Log file.
ceDigi Log file.
muDauSteps Log file.
ceMix Log file.
rootOverlaps Log file.
g4surfaceCheck Log file.
trigger Log file.
check_cmake Log file.
FIXME, TODO TODO (0) FIXME (0) in 1 files
clang-tidy 0 errors 0 warnings
whitespace check no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at e304eab after being merged into the base branch at 329c100.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants