samreid
commented
3 months ago @zepumph and I will investigate this
Closed pixelzoom closed 3 months ago
samreid
commented
3 months ago @zepumph and I will investigate this
zepumph
commented
3 months ago Likely similar to the current cck problem. from observable array state changes. Ill take a look.
zepumph
commented
3 months ago Seems related to https://github.com/phetsims/phet-io/issues/1661. I'll fix it up next week. Thanks for the report.
The assertion that fails things is:
[CONSOLE] Assertion failed: counts out of sync
That is right where we were working in that issue. My best guess right now is that we will need a solution similar to
Since now there is time between when ObservableArrays have their new values set, and then later when notifications about them occur.
pixelzoom
commented
3 months ago My best guess right now is that we will need a solution similar to
How timely... and unfortunate. I'm working on instrumenting particles in gas-properties, and (less than 5 minutes ago) was scratching my head about that very bit of code. I still don't understand it, why it's only needed (or present, anyway) in 1 of 2 particle systems, and I'll need to do a deep dive into the issue noted in the code comments, https://github.com/phetsims/gas-properties/issues/178.
zepumph
commented
3 months ago @samreid and my proposal is this, but we want to sync discuss with @pixelzoom before committing.
FYI issue renamed after changes from https://github.com/phetsims/phet-io/issues/1986
zepumph
commented
3 months ago Error: Assertion failed: counts out of sync
at window.assertions.assertFunction (http://128.138.93.172/continuous-testing/ct-snapshots/1715025307059/assert/js/assert.js:28:13)
at assert (createBunnyArray.ts:67:14)
at listener (TinyEmitter.ts:213:6)
at notifyLoop (TinyEmitter.ts:196:13)
at emit (Emitter.ts:64:21)
at emit (createObservableArray.ts:169:58)
at (createObservableArray.ts:502:38)
at setNotificationsDeferred (createObservableArray.ts:318:26)
at listener (TinyEmitter.ts:213:6)
at notifyLoop (TinyEmitter.ts:196:13)natural-selection : migration : 1.5->main : assert
http://128.138.93.172/continuous-testing/ct-snapshots/1715025307059/phet-io-wrappers/migration/?sim=natural-selection&oldVersion=1.5&phetioMigrationReport=assert&locales=*&phetioDebug=true&phetioWrapperDebug=true&fuzz&migrationRate=5000&&wrapperContinuousTest=%7B%22test%22%3A%5B%22natural-selection%22%2C%22migration%22%2C%221.5-%3Emain%22%2C%22assert%22%5D%2C%22snapshotName%22%3A%22snapshot-1715025307059%22%2C%22timestamp%22%3A1715025960284%7D
Assertion failed: counts out of sync
Error: Assertion failed: counts out of sync
at window.assertions.assertFunction (http://128.138.93.172/continuous-testing/ct-snapshots/1715025307059/assert/js/assert.js:28:13)
at assert (createBunnyArray.ts:67:14)
at listener (TinyEmitter.ts:213:6)
at notifyLoop (TinyEmitter.ts:196:13)
at emit (Emitter.ts:64:21)
at emit (createObservableArray.ts:169:58)
at (createObservableArray.ts:502:38)
at setNotificationsDeferred (createObservableArray.ts:318:26)
at listener (TinyEmitter.ts:213:6)
at notifyLoop (TinyEmitter.ts:196:13)
[URL] http://128.138.93.172/continuous-testing/aqua/html/wrapper-test.html?url=..%2F..%2Fct-snapshots%2F1715025307059%2Fphet-io-wrappers%2Fmigration%2F%3Fsim%3Dnatural-selection%26oldVersion%3D1.5%26phetioMigrationReport%3Dassert%26locales%3D*%26phetioDebug%3Dtrue%26phetioWrapperDebug%3Dtrue%26fuzz%26migrationRate%3D5000%26&duration=80000&testInfo=%7B%22test%22%3A%5B%22natural-selection%22%2C%22migration%22%2C%221.5-%3Emain%22%2C%22assert%22%5D%2C%22snapshotName%22%3A%22snapshot-1715025307059%22%2C%22timestamp%22%3A1715025960284%7D
[NAVIGATED] http://128.138.93.172/continuous-testing/aqua/html/wrapper-test.html?url=..%2F..%2Fct-snapshots%2F1715025307059%2Fphet-io-wrappers%2Fmigration%2F%3Fsim%3Dnatural-selection%26oldVersion%3D1.5%26phetioMigrationReport%3Dassert%26locales%3D*%26phetioDebug%3Dtrue%26phetioWrapperDebug%3Dtrue%26fuzz%26migrationRate%3D5000%26&duration=80000&testInfo=%7B%22test%22%3A%5B%22natural-selection%22%2C%22migration%22%2C%221.5-%3Emain%22%2C%22assert%22%5D%2C%22snapshotName%22%3A%22snapshot-1715025307059%22%2C%22timestamp%22%3A1715025960284%7D
[ATTACHED]
[NAVIGATED] about:blank
[NAVIGATED] http://128.138.93.172/continuous-testing/ct-snapshots/1715025307059/phet-io-wrappers/migration/?sim=natural-selection&oldVersion=1.5&phetioMigrationReport=assert&locales=*&phetioDebug=true&phetioWrapperDebug=true&fuzz&migrationRate=5000&&wrapperContinuousTest=%7B%22test%22%3A%5B%22natural-selection%22%2C%22migration%22%2C%221.5-%3Emain%22%2C%22assert%22%5D%2C%22snapshotName%22%3A%22snapshot-1715025307059%22%2C%22timestamp%22%3A1715025960284%7D
[ATTACHED]
[NAVIGATED] about:blank
[ATTACHED]
[NAVIGATED] about:blank
[CONSOLE] enabling assert
[NAVIGATED] http://128.138.93.172/continuous-testing/ct-snapshots/1715025307059/studio/?sim=natural-selection&phetioWrapperDebug=true&wrapperContinuousTest=%7B%22test%22%3A%5B%22natural-selection%22%2C%22migration%22%2C%221.5-%3Emain%22%2C%22assert%22%5D%2C%22snapshotName%22%3A%22snapshot-1715025307059%22%2C%22timestamp%22%3A1715025960284%7D&phetioMigrationReport=assert&fuzz&locales=*
[ATTACHED]
[NAVIGATED] about:blank
[CONSOLE] enabling assert
[NAVIGATED] about:blank
[NAVIGATED] http://128.138.93.172/continuous-testing/ct-snapshots/1715025307059/natural-selection/natural-selection_en.html?brand=phet-io&ea&postMessageOnError&sim=natural-selection&phetioWrapperDebug=true&wrapperContinuousTest=%7B%22test%22%3A%5B%22natural-selection%22%2C%22migration%22%2C%221.5-%3Emain%22%2C%22assert%22%5D%2C%22snapshotName%22%3A%22snapshot-1715025307059%22%2C%22timestamp%22%3A1715025960284%7D&phetioMigrationReport=assert&fuzz&locales=*&phetioEmitAPIBaseline&phetioCreateArchetypes&phetioEmitHighFrequencyEvents=false
[CONSOLE] enabling assert
[NAVIGATED] https://phet-io.colorado.edu/sims/natural-selection/1.5/wrappers/studio/?phetioWrapperDebug=true&wrapperContinuousTest=%7B%22test%22%3A%5B%22natural-selection%22%2C%22migration%22%2C%221.5-%3Emain%22%2C%22assert%22%5D%2C%22snapshotName%22%3A%22snapshot-1715025307059%22%2C%22timestamp%22%3A1715025960284%7D&phetioMigrationReport=assert&fuzz&exposeStandardPhetioWrapper
[CONSOLE] enabling assert
[ATTACHED]
[NAVIGATED] about:blank
[NAVIGATED] https://phet-io.colorado.edu/sims/natural-selection/1.5/natural-selection_all_phet-io_debug.html?ea&postMessageOnError&phetioWrapperDebug=true&wrapperContinuousTest=%7B%22test%22%3A%5B%22natural-selection%22%2C%22migration%22%2C%221.5-%3Emain%22%2C%22assert%22%5D%2C%22snapshotName%22%3A%22snapshot-1715025307059%22%2C%22timestamp%22%3A1715025960284%7D&phetioMigrationReport=assert&fuzz&exposeStandardPhetioWrapper&phetioEmitAPIBaseline&phetioCreateArchetypes&phetioEmitHighFrequencyEvents=false
[CONSOLE] enabling assert
[CONSOLE] continuous-test-wrapper-load
[CONSOLE] Assertion failed: counts out of sync
[PAGE ERROR] Error: Error: Assertion failed: counts out of sync
at window.assertions.assertFunction (http://128.138.93.172/continuous-testing/ct-snapshots/1715025307059/assert/js/assert.js:28:13)
at http://128.138.93.172/continuous-testing/ct-snapshots/1715025307059/chipper/dist/js/natural-selection/js/common/model/createBunnyArray.js:49:15
at TinyEmitter.notifyLoop (http://128.138.93.172/continuous-testing/ct-snapshots/1715025307059/chipper/dist/js/axon/js/TinyEmitter.js:176:7)
at TinyEmitter.emit (http://128.138.93.172/continuous-testing/ct-snapshots/1715025307059/chipper/dist/js/axon/js/TinyEmitter.js:161:14)
at Emitter.emit (http://128.138.93.172/continuous-testing/ct-snapshots/1715025307059/chipper/dist/js/axon/js/Emitter.js:52:22)
at http://128.138.93.172/continuous-testing/ct-snapshots/1715025307059/chipper/dist/js/axon/js/createObservableArray.js:111:58
at Proxy.setNotificationsDeferred (http://128.138.93.172/continuous-testing/ct-snapshots/1715025307059/chipper/dist/js/axon/js/createObservableArray.js:419:37)
at http://128.138.93.172/continuous-testing/ct-snapshots/1715025307059/chipper/dist/js/axon/js/createObservableArray.js:240:27
at TinyEmitter.notifyLoop (http://128.138.93.172/continuous-testing/ct-snapshots/1715025307059/chipper/dist/js/axon/js/TinyEmitter.js:176:7)
at TinyEmitter.emit (http://128.138.93.172/continuous-testing/ct-snapshots/1715025307059/chipper/dist/js/axon/js/TinyEmitter.js:161:14)
[CONSOLE] continuous-test-wrapper-error
id: "Sparky Node Puppeteer"
I think I finally cracked this, thanks for help from @samreid and @pixelzoom. We needed more objects to be phetioState:true and handle their own state, since we could no longer depend on ObservableArray to fire notifications eagerly to update them during state set. I'll check back on CT later.
zepumph
commented
3 months ago CT is happy in the state wrapper, but is complaining about two more things:
{ activeProperty: { value: true } }. This involves clearing arrays without resetting them. I'll need to figure out how to do a better unit test hack. zepumph
commented
3 months ago @samreid and I discussed (1), and think the next step is a custom migration rule that adds the bunny lists and countsProperties into the state manually, by looking at the list of bunnies in the PhetioGroup state.
zepumph
commented
3 months ago Unit tests fixed above, all that is left is to handle migration
zepumph
commented
3 months ago Pretty close to a test point here.
zepumph
commented
3 months ago Good progress plus a bug fix for Migration Report processing bug.
pixelzoom
commented
3 months ago @zepumph Please elaborate (in the code comments) on the change shown below that you made in https://github.com/phetsims/natural-selection/commit/db44f84fa0a55ad86b3b7a66599d6acc6c114b8f. This is something I have never encountered, I don't understand it, and I could not maintain it.
// State may set in an unexpected order, but by the end, we must have the right counts.
if ( assert && Tandem.PHET_IO_ENABLED ) {
phet.phetio.phetioEngine.phetioStateEngine.stateSetEmitter.addListener( ( state: PhetioState, scopeTandem: Tandem ) => {
if ( countsProperty.tandem.hasAncestor( scopeTandem ) ) {
assert && assert( countsProperty.value.totalCount === bunnyArray.length, 'counts out of sync' );
}
} );
}I tracked scopeTandem down to this code in PhetioStateEngine.ts. It's doc makes no sense to me -- "returns if... "? This method's return is void. I also have no idea what a "partial state" is, or what it means to be "in the scope of a partial state".
/**
* Sets the state from an object that represents the state.
*
* @param state - a json object (not a string) that specifies the values to set
* @param scopeTandem - returns if the given Tandem is in the scope of a (partial) state
*/
public setState( state: FullPhetioState, scopeTandem: Tandem ): void {Updated doc above. I originally thought that would be useful to add so that we didn't assert for all bunnyCollections if we only just setState on a single screen (like when pressing the resetAllButton in a standard PhET-iO Wrapper from studio), but I don't actually think it is necessary. I'd be happy to delete it for this pattern generally, because I don't mind an assertion that says, "if you just set another screen, my counts should still be correct".
All other work for this issue has been done. I found a bug over in https://github.com/phetsims/phet-io-wrappers/issues/641 that has been fixed, but isn't effecting NS migration after the above commits. CT is clear, and I'll go ahead and commit the removal of the scopeTandem stuff now.
zepumph
commented
3 months ago @pixelzoom feel free to close or reach out to discuss anything further.
phet-dev
commented
3 months ago Reopening because there is a TODO marked for this issue.
zepumph
commented
3 months ago What a fun bug in the TODO checker. Here is the comment:
/**
* In https://github.com/phetsims/natural-selection/issues/361, elements were set to phetioState:true:
* liveBunnies
* deadBunnies
* liveBunnies.countsProperty
* deadBunnies.countsProperty
*
* These used to be derived from the bunnies themselves in the sim, and so this rule derives these values from the
* bunny states, and creates the needed state for these derived stateful values.
*
* NOTE: IntentionalAny is used quite a bit here, to prevent importing over into the main sim codebase. Be careful!
*
* TODO: Handle recessiveMutants?, https://github.com/phetsims/natural-selection/issues/362
*/
The TODO is for another issue. Closing
zepumph
commented
3 months ago This is still failing because we need to create some default for recessiveMutants, so that it actually is getting set. This will mostly be a stop gap until we determine how to proceed with https://github.com/phetsims/natural-selection/issues/362, so I'll keep that TODO in place.
zepumph
commented
3 months ago This commit adds all recessive mutants into the migrated stateful list, even if they have already been mated with. As I was reviewing these changes, I was wondering if I should have stop-gapped by just having an empty list/counts in the state. Anywho, let's see how CT does tonight.
zepumph
commented
3 months ago Ok excellent. This has fixed the assertion, though it doesn't transfer state of the recessiveMutants or anything (see other issue for that).
pixelzoom
commented
3 months ago Reopening.
This commit adds all recessive mutants into the migrated stateful list, even if they have already been mated with.
That will result in incorrect behavior. Some recessive mutants may "mate eagerly" twice, resulting in a significantly diffferent population that displays more of the recessive gene than it should.
For correct behavior: For each bunny that passes isRecessiveMutant, add it to recessiveMutants if no other bunny has the mutant as its father or mother.
zepumph
commented
3 months ago That makes a lot of sense! There is a TODO in this processor to support recessveMutants over in https://github.com/phetsims/natural-selection/issues/362. In that issue we will decide how important it is to fix this state bug in older version of NS, and how to support migration with it. Reclosing
This CT error started at 5/1/2024 2:58:57pm. It's present on every CT cycle since then. I'm still unable to understand this "mismatched index" type of error, so assigning to @zepumph for assistance.