Skip to content

Add reproducer for stochastic heat transfer - #88

Open
MarcCote with Copilot wants to merge 6 commits into
mainfrom
copilot/fix-heat-transfer-stochastic-issue
Open

Add reproducer for stochastic heat transfer#88
MarcCote with Copilot wants to merge 6 commits into
mainfrom
copilot/fix-heat-transfer-stochastic-issue

Conversation

Copilot AI commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Identical task-1-boil runs can produce different outcomes, with steam intermittently missing after the reported action sequence.

  • Reproduction
    • Adds scripts/reproduce_heat_transfer_stochasticity.py.
    • Replays the scenario eight times in one environment instance.
    • Reports steam presence per run and the total failures.
python scripts/reproduce_heat_transfer_stochasticity.py

Copilot AI linked an issue Aug 4, 2026 that may be closed by this pull request
Co-authored-by: MarcCote <660004+MarcCote@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix non-determinism in heat transfer outcome Add reproducer for stochastic heat transfer Aug 4, 2026
Copilot AI requested a review from MarcCote August 4, 2026 17:17
@MarcCote

MarcCote commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

@copilot would the fix proposed in #85 solves this? If so, would there be any side-effects we should be aware of?

Copilot AI commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

@copilot would the fix proposed in #85 solves this? If so, would there be any side-effects we should be aware of?

Yes for the nondeterminism, but not for the expected outcome. I ran this PR’s exact 8-run reproducer against both JARs: the current JAR had steam missing in 5/8 runs, while PR #85’s JAR had steam missing in 8/8 runs (and it remained water for six additional ticks). The EnvObject.hashCode = uuid change fixes the existing equals/hashCode contract violation and stabilizes Set[EnvObject] traversal after resets, so it removes the stochastic branch; it deterministically selects the no-steam trajectory.

The main side effect is global: every hash-set traversal involving EnvObject changes, including physics tick order, combustion’s choice of water, object/reference matching, and ambiguous selection. Existing trajectories or gold actions could therefore change. The UUID is immutable, so the hash implementation itself is sound, but hash-set iteration is still not an explicit ordering guarantee across Scala/JVM versions; sorting by UUID (or using an ordered collection) would be stronger where order affects physics. PR #85 also upgrades Scala/SBT and rebuilds the JAR; those changes are not required for the logical fix and broaden the compatibility/build surface, so I would isolate them if possible.

@MarcCote

MarcCote commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

@copilot Come up with a stronger fix (keep it as minimal as possible) that would be stable across Scale/JVM, then.

Co-authored-by: MarcCote <660004+MarcCote@users.noreply.github.com>

Copilot AI commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Come up with a stronger fix (keep it as minimal as possible) that would be stable across Scale/JVM, then.

Implemented in ae39d37. Order-sensitive simulation paths now explicitly sort objects by immutable UUID before heat transfer, recursive ticking, portal ticking, and combustion’s first-water selection, without changing global hash/collection behavior. The rebuilt JAR reports steam in 8/8 reproducer runs (0 missing); the new regression and existing observation-determinism test both pass.

@MarcCote

MarcCote commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

@copilot and fix in ae39d37 covers all the cases in the codebase where ordered-matter enumeration is done? Really?

Co-authored-by: MarcCote <660004+MarcCote@users.noreply.github.com>

Copilot AI commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

@copilot and fix in ae39d37 covers all the cases in the codebase where ordered-matter enumeration is done? Real...

No—ae39d37 only covered the reproducer’s paths. The audit found additional order-sensitive enumeration in bee/pollination behavior, first-match scans, parser/action ambiguity handling, observations/JSON, and seeded shuffles. Fixed in e7395e7: all EnvObject collection accessors now return UUID-ordered sets, parser/action flattening preserves that order, and the few order-losing random selections are explicitly sorted. Order-insensitive membership/count uses remain unchanged. The Scala build, all 9 tests, lint/hooks, and CodeQL pass.

@MarcCote

MarcCote commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

@copilot be systematic about it, and check the whole codebase. Produce a Github check list in your response.

Copilot AI and others added 2 commits August 4, 2026 19:07
Co-authored-by: MarcCote <660004+MarcCote@users.noreply.github.com>
Co-authored-by: MarcCote <660004+MarcCote@users.noreply.github.com>

Copilot AI commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

@copilot be systematic about it, and check the whole codebase. Produce a Github check list in your response.

Addressed in 9045cb2 and 9cf9a30.

  • Audited all 182 Scala, 13 Python, and shell/build/config sources.
  • Audited every object-producing boundary: primary and legacy containment, recursive traversal, portals, electrical connections, and goal monitors.
  • Audited order-sensitive consumers: physics ticks, heat transfer, combustion, pollination, indexed/first-match selection, task setup, and gold-path generation.
  • Canonicalized object order by immutable UUID; added lexical/UUID tie-breakers for partial sorts.
  • Canonicalized parser/action enumeration, referents, ambiguity choices, and trigger-argument object IDs.
  • Canonicalized observations, object trees, all public object/type JSON maps, referent arrays, and the persisted type-ID table.
  • Audited the separate legacy language runtime: containment, rule parameters, taxonomy traversal, inherited properties, and diagnostics.
  • Audited random selections/shuffles; object inputs are ordered before deliberate randomness. Remaining unordered collection use is membership/count/key lookup, not object selection or sequencing.
  • Added exact snapshot checks across 8 resets and 2 independent JVMs.
  • Verified all 9 tests, Flake8, all pre-commit hooks, CodeQL (0 alerts), and the reproducer (8/8 steam, 0/8 missing).

@MarcCote

MarcCote commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

@copilot what is the speed impact of the proposed changes?

Copilot AI commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

@copilot what is the speed impact of the proposed changes?

Measured 9cf9a30 against the pre-PR JAR (c6edd47) on OpenJDK 17, pinned to one core, after JVM warm-up, with interleaved baseline/current samples.

  • Raw Scala-heavy ticks do regress: the 30-task sweep’s median was +0.10 ms/tick (+15.7%); a 100-sample run on the largest world was 0.637 → 0.710 ms/tick (+11.4%, 95% CI +10.0–14.1%).
  • The exact five-action boiling trajectory had no measurable raw-step regression (55.0 → 54.2 ms total; CI includes zero).
  • Normal Python env.step() usage had no measurable slowdown: pooled across all 30 tasks it was 26.57 → 26.24 ms/step (the apparent 1.2% speedup is noise), and the boiling trajectory’s public path was statistically unchanged.
  • Loading/resetting all 30 tasks was also unchanged (2.714 → 2.656 s, CI includes zero).
  • Newly ordered metadata APIs add roughly 0.06–0.1 ms/call; object-tree generation adds about 0.6 ms.

The internal cost comes from rebuilding temporary UUID TreeSets (O(n log n)) in containment accessors, sometimes followed by a redundant sort. Thus direct Scala/server users should expect roughly a 10–16% raw-tick hit, while normal Python API users should see effectively 0% end-to-end impact because Py4J/API bookkeeping dominates. If the raw-tick overhead is a concern, the first minimal optimization would be to remove sorts of already UUID-ordered sets; avoiding repeated set reconstruction would require a somewhat broader collection-storage change.

@MarcCote

MarcCote commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

@copilot Are the tasks generated before and after the proposed changes the same for a given variation and seed?

Copilot AI commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Are the tasks generated before and after the proposed changes the same for a given variation and seed?

Semantically yes; byte-for-byte no. variationIdx is also the RNG seed (there is no separate task seed). I compared c6edd47 with 9cf9a30 for all 7,207 valid task/variation pairs: all had identical task descriptions, normalized goals, and canonical initial object trees—including UUIDs, containment, properties, and agent location. The available referent multisets were also identical. Raw JSON differs intentionally because it is now ordered; in 92 cases, 167 referent strings move only among duplicate objects with the same name/type/container. The rebuilt JAR also drops whitespace padding in goal-progress diagnostics due modern-JDK String.formatted resolution, but the goal content is unchanged. Gold paths and post-action physics trajectories may differ because those are the order-sensitive behaviors this PR changes.

@MarcCote
MarcCote marked this pull request as ready for review August 5, 2026 13:17
@MarcCote

MarcCote commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

@copilot looking at the old code, why was the steam only produced half the time?

Copilot AI commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@copilot looking at the old code, why was the steam only produced half the time?

It was a tick-order race, not probabilistic heat transfer:

  • Igniting the drawer made Combustion.combustionTick set the sink’s liquid water to 550°C.
  • EnvObject.tick recursively visited the sink’s children through an unordered mutable.Set.
  • Drawer first: it heated the water, then the water tick ran StateOfMatter.ChangeOfState, renamed it to steam, and the liquid-only drain left it in the sink.
  • Water first: its phase check ran while it was still 10°C; the drawer then heated it to 550°C, but it could not tick again that step. The drain still saw its state as liquid and moved it to the sewer. The final look around is a free action, so it did not advance another tick.

Across resets, UUIDs were repeatable, but EnvObject defined UUID-based equals without a matching hashCode; fresh objects therefore got different identity hashes and different hash-set traversal orders. So “half” was only the observed split, not a true 50% RNG probability. I confirmed the old JAR’s 3 successful runs had 550°C steam in the sink, while all 5 failures had 550°C liquid water in the sewer.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Heat transfer is stochastic

2 participants