Add graceful option for Property/Emitter

[samreid]

In https://github.com/phetsims/phet-io/issues/1881#issuecomment-1255667686 we discussed that (at least for PhET-iO), localeProperty should behave gracefully when trying to set a non-existant locale. If that behavior is also deemed acceptable for PhET brand, then it would make sense to add a Property option graceful which has the following behavior:

• Check isValidValue before setting the value. If the value is valid, proceed with the set (could disable the post-set assertion if we want to save a CPU cycle). If the value is invalid, do not try to set the value at all. This would need to work whether or not assertions are enabled. (So we would have to make sure we are not stripping out validators, which I don't think we are).

[samreid]

@arouinfar and @kathy-phet and I discussed this today, and were surprised to see that

const x = new NumberProperty( 0, {
  range: new Range( -10, 10 ),
  tandem: Tandem.GENERAL_MODEL.createTandem( 'myAwesomeNumberProperty' )
} );
x.value = 11;

Ended up with x.value = 11. Same with

phetioClient.invoke( 'gravityAndOrbits.general.model.myAwesomeNumberProperty', 'setValue', [ 11 ] );

We also discussed that a graceful method would take more CPU. We could say that graceful only defaults to true for phet-io instrumented things. Or maybe always defaults to graceful: true. But @kathy-phet thinks the main benefit may be around phet-io.

We would like to raise this to a dev team discussion. Before that, @samreid could run performance testing.

@kathy-phet also indicated that @zepumph has been instrumental in building the validation library, and his input would be valuable here.

[liammulh]

In dev meeting:

• SR: Do we want to add an earlier guard?
• JB: Why can't it always be graceful?
• SR: Performance, basically.
• JB: Is this here for debugging or PhET-iO?
• SR: If you set a bad value on an axon property, do you want to know about it?
• JB: I don't want to know if assertions are off.
• JB: From the external usage perspective, I'd like them to know they are setting a bad value.
• SR: Even in front of students?
• JB: Do 3P PhET-iO devs use assertions?
• SR: Yes.
• JB: Okay, it seems reasonable tradeoff to turn off the validators in built mode.
• SR: If 11 is out of range, it should never be set to 11.
• SR: We don't know how much of a performance hit adding validators would be... If it's not too bad, we should add them.

We need some data on performance impact of adding validators. Back to @samreid.

• If assertions enabled, throw an error.
• If assertions not enabled, it should be a no-op.

[samreid]

I'd also like to request @zepumph's opinion on this issue (even before we have performance data).

[zepumph]

I'm unsure exactly what I'm commenting on. In built versions when assertions are not enabled, we have long had the philosophy to do the best we can, so the example in https://github.com/phetsims/axon/issues/419#issuecomment-1256516874 makes sense if validation isn't occurring.

If we want to have a property where we turn validation off, don't we already have that ability by just using a Property without validation supplied? Or specifying our own validation that specifically ignores the parts we need to be graceful to?

For localeProperty, the main issue seems to be that we want Studio have good UI, but also for validValues to be a no-op (https://github.com/phetsims/joist/blob/9ddbe2ac36d02add2b7e0385f47dc9a74dac085a/js/i18n/localeProperty.ts#L55). The solution there is to fix Studio's UI, not to redefine a way of supplying validation metadata to LocaleProperty for it to specifically be ignored.

Happy to discuss further! I'm sure I'm missing something.

[samreid]

Here's a patch for testing:

```diff Index: js/ReadOnlyProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/ReadOnlyProperty.ts b/js/ReadOnlyProperty.ts --- a/js/ReadOnlyProperty.ts (revision df075961d21fd59d84f7ecd9a5552e9e3ff13a18) +++ b/js/ReadOnlyProperty.ts (date 1665434178211) @@ -18,7 +18,6 @@ import PropertyStatePhase from './PropertyStatePhase.js'; import TinyProperty from './TinyProperty.js'; import units from './units.js'; -import validate from './validate.js'; import TReadOnlyProperty, { PropertyLazyLinkListener, PropertyLinkListener, PropertyListener } from './TReadOnlyProperty.js'; import optionize from '../../phet-core/js/optionize.js'; import Validation, { Validator } from './Validation.js'; @@ -202,9 +201,6 @@ Validation.validateValidator( this.valueValidator ); - // validate the initial value as well as any changes in the future - this.link( ( value: T ) => validate( value, this.valueValidator, VALIDATE_OPTIONS_FALSE ) ); - if ( Tandem.PHET_IO_ENABLED && this.isPhetioInstrumented() && this.phetioState && phetioAPIValidation.enabled ) { assert && assert( options.phetioValueType !== IOType.ObjectIO, 'Stateful PhET-iO Properties must specify a phetioValueType: ' + this.phetioID ); } @@ -258,9 +254,23 @@ this.hasDeferredValue = true; } else if ( !this.equalsValue( value ) ) { - const oldValue = this.get(); - this.setPropertyValue( value ); - this._notifyListeners( oldValue ); + + // TODO https://github.com/phetsims/axon/issues/419 should this check be moved to setPropertyValue? + // TODO https://github.com/phetsims/axon/issues/419: If so, should the equalsValue check move there too? + const validationError = Validation.getValidationError( value, this.valueValidator, VALIDATE_OPTIONS_FALSE ); + + if ( validationError ) { + assert && assert( false, `Validation error: ${validationError}` ); + + // NOTE: Do not try to set an invalid value + } + else { + + // Value is valid, set the value and update listeners. + const oldValue = this.get(); + this.setPropertyValue( value ); + this._notifyListeners( oldValue ); + } } } } ```

Let's try at least 2 sims. Build and unbuilt. With and without assertions. Different browsers. Different platforms. With and without the patch. Maybe measure time to LCP? https://web.dev/lcp/?utm_source=devtools And some way of measuring performance as the sim animates? Throw away first run of each set. Use T-test for independent means: https://www.socscistatistics.com/tests/studentttest/default2.aspx

Memory is not a concern since the validator is stored whether assertions are enabled or not. The patch may actually reduce memory since it decreases the number of closures by N, where N is the number of ReadOnlyProperty instances.

[samreid]

Results of 1st experiment, copy/pasted from excel. Times are in milliseconds, smaller is faster=better.

<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:x="urn:schemas-microsoft-com:office:excel" xmlns="http://www.w3.org/TR/REC-html40">

sim | Gravity and Orbits |   -- | -- | -- built? | Unbuilt |   browser | OSX/Chrome |   assertions | Without assertions |   condition | WITHOUT PATCH | WITH PATCH timing | LCP | LCP run 1 | 1681.9 | 1725.2 run 2 | 1692.8 | 1719.4 run 3 | 1697.8 | 1724.6 run 4 | 1646.6 | 1764.8 run 5 | 1709.5 | 1724.7

For a one-sided t-test:

The t-value is -3.38572. The p-value is .00478. The result is significant at p < .05.

This means that for this experimental condition, validating with assertions disabled is statistically significantly slower for sim startup (46ms slower on average).

[samreid]

Here is the same test but with built sims. Surprisingly, the mean for the guarded code is faster, but not statistically significantly so:

Built Gravity and Orbits, assertions are stripped out, Mac OS/Chrome

<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:x="urn:schemas-microsoft-com:office:excel" xmlns="http://www.w3.org/TR/REC-html40">

WITHOUT PATCH | WITH PATCH -- | -- 1418 | 1567.2 1472.2 | 1412.6 1445.1 | 1427.7 1441.6 | 1423.8 1595.4 | 1446.5

The t-value is 0.44587. The p-value is .333757. The result is not significant at p < .05.

[samreid]

I observed that gravity and orbits is running that validation 2330 times during startup. But it is running many more times during animation and interaction. Animating gravity and orbits for 10 seconds runs that validator 23600 times. This looks like about 40 times per frame.

[samreid]

Sampling over 35000ms in a built debug version of gravity and orbits, it says 68ms were sampled in getValidationError. That is 0.2% of the total time. I'm feeling that is an acceptably low level. I feel this issue is at a good point to check in with @zepumph, perhaps at an upcoming zoom meeting.

[samreid]

@zepumph and I discussed this and considered the following scenario:

aProperty = 0 bProperty = 0 cProperty = aProperty + bProperty (limited between 0 and 10)

Then set aProperty = 5. Then set bProperty = 6.

At this point, we have 5+6 = 5, which is not desirable. So @zepumph recommends that we do not commit this extra guard, because it could yield equally inconsistent states.

@zepumph also indicated that we may want to capture top-level API calls and guard them in this way. We have a guard in the studio UI to validate values before sending them to the sim. Why not also have a guard for setValue calls that validates, and returns an error if the validation failed?

[samreid]

@zepumph and I wrote this patch, we like the PropertyIO guard and want to do the same for EmitterIO and PhetioActionIO.

```diff Index: main/axon/js/Emitter.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/axon/js/Emitter.ts b/main/axon/js/Emitter.ts --- a/main/axon/js/Emitter.ts (revision b9ff232bf30840685bcd66b642f247d789243ee1) +++ b/main/axon/js/Emitter.ts (date 1665516520287) @@ -19,6 +19,7 @@ import TinyEmitter from './TinyEmitter.js'; import Tandem from '../../tandem/js/Tandem.js'; import TEmitter, { TEmitterListener, TEmitterParameter } from './TEmitter.js'; +import Validation from './Validation.js'; // By default, Emitters are not stateful const PHET_IO_STATE_DEFAULT = false; @@ -178,7 +179,17 @@ parameterTypes: parameterTypes, // Match `Emitter.emit`'s dynamic number of arguments - implementation: Emitter.prototype.emit, + implementation: function( this: Emitter, ...values: unknown[] ) { + + const validationError = Validation.getValidationError( value, this.valueValidator, VALIDATE_OPTIONS_FALSE ); + + if ( validationError ) { + throw new Error( `Validation error: ${validationError}` ); + } + else { + this.set( value ); + } + }, documentation: 'Emits a single event to all listeners.', invocableForReadOnlyElements: false } Index: main/tandem/js/PhetioDataHandler.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/tandem/js/PhetioDataHandler.ts b/main/tandem/js/PhetioDataHandler.ts --- a/main/tandem/js/PhetioDataHandler.ts (revision 3525d0af79bc64bd4f667d26136e04fa315dddd4) +++ b/main/tandem/js/PhetioDataHandler.ts (date 1665516607647) @@ -175,6 +175,24 @@ } } + /** + * Validate that provided args match the expected schema given via options.parameters. + */ + protected getValidationErrors( ...args: T ): void { + assert && assert( args.length === this.parameters.length, + `Emitted unexpected number of args. Expected: ${this.parameters.length} and received ${args.length}` + ); + for ( let i = 0; i < this.parameters.length; i++ ) { + const parameter = this.parameters[ i ]; + assert && validate( args[ i ], parameter, VALIDATE_OPTIONS_FALSE ); + + // valueType overrides the phetioType validator so we don't use that one if there is a valueType + if ( parameter.phetioType && !parameter.valueType ) { + assert && validate( args[ i ], parameter.phetioType.validator, VALIDATE_OPTIONS_FALSE ); + } + } + } + /** * Gets the data that will be emitted to the PhET-iO data stream, for an instrumented simulation. * @returns the data, keys dependent on parameter metadata Index: main/axon/js/ReadOnlyProperty.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/axon/js/ReadOnlyProperty.ts b/main/axon/js/ReadOnlyProperty.ts --- a/main/axon/js/ReadOnlyProperty.ts (revision b9ff232bf30840685bcd66b642f247d789243ee1) +++ b/main/axon/js/ReadOnlyProperty.ts (date 1665516368781) @@ -576,9 +576,17 @@ setValue: { returnType: VoidIO, parameterTypes: [ parameterType ], + implementation: function( this: ReadOnlyProperty, value: T ) { - // @ts-ignore - implementation: ReadOnlyProperty.prototype.set, + const validationError = Validation.getValidationError( value, this.valueValidator, VALIDATE_OPTIONS_FALSE ); + + if ( validationError ) { + throw new Error( `Validation error: ${validationError}` ); + } + else { + this.set( value ); + } + }, documentation: 'Sets the value of the Property. If the value differs from the previous value, listeners are ' + 'notified with the new value.', invocableForReadOnlyElements: false ```

I'll write an initial draft and request review from @zepumph.

[samreid]

@marlitas and I implemented this yesterday and felt reasonably confident about it, so we committed it to master. We followed the precedent from the patch about leveraging phetioType validation where possible -- we were less certain about that part. @zepumph can you please review?

[zepumph]

RE: https://github.com/phetsims/tandem/commit/112e3b29a8d3e0d5c759981870466fa0252923d1#diff-33850d210557136dc4ace95e99c3236a9016399cf89e8ca1a05a789e412998daR189

would it be better to emit4 and return [ error, null, null, error], or [error, error]? In PhET-iO we decided the first one for giving errors/returnValues back to the wrapper. I believe that is more clear.

I'm worried that there is no "opt out for perforamance" feature of this. When we originally created the getValidationError functionality, we purposefully didn't create this block, instead recognizing that if the user wanted more info about what would fail, they could have it, but we didn't want that necessarily leaking in to production wrapper code.

https://github.com/phetsims/axon/blob/e24d4a177d78b60f3470860d9eeb4fa7b5009716/js/ReadOnlyProperty.ts#L576-L589 This is why you see the overly-complicated call in PhetioElementView.doTheInvocation:

https://github.com/phetsims/studio/blob/019d611ecbbdf8287ee4f34b4fe9d2eea771a44f/js/PhetioElementView.ts#L113

We followed the precedent from the patch about leveraging phetioType validation where possible

I believe that this code was written in Emitter BEFORE ValidatorDef included phetioType. So we had to manually check on that validator too. My recommendation would be to remove that, and write into tests ensuring that the phetioType of the parameter is still validated. It should be an easy-enough thing to ensure. I can assist if you'd like, but I'd at least want to pair with you.

[samreid]

I feel collaboration would be most efficient for this issue. I'll reach out on slack.

[samreid]

Your recommendations are all great, I implemented them in this patch, and threw in a unit test for the phetioType part:

```diff Subject: [PATCH] Add get/set/copy buttons div, make buttons same height, remove br, specify font size, update tooltips, see https://github.com/phetsims/studio/issues/298 --- Index: main/tandem/js/PhetioDataHandler.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/tandem/js/PhetioDataHandler.ts b/main/tandem/js/PhetioDataHandler.ts --- a/main/tandem/js/PhetioDataHandler.ts (revision cba9a2792976fb7c6d44f3af71af56ae62129d52) +++ b/main/tandem/js/PhetioDataHandler.ts (date 1678288859068) @@ -175,23 +175,13 @@ /** * Validate that provided args match the expected schema given via options.parameters. */ - protected getValidationErrors( ...args: T ): Array { + public getValidationErrors( ...args: T ): Array { assert && assert( args.length === this.parameters.length, `Emitted unexpected number of args. Expected: ${this.parameters.length} and received ${args.length}` ); - const errors = []; - for ( let i = 0; i < this.parameters.length; i++ ) { - const parameter = this.parameters[ i ]; - let error = Validation.getValidationError( args[ i ], parameter, VALIDATE_OPTIONS_FALSE ); - error !== null && errors.push( error ); - - // valueType overrides the phetioType validator so we don't use that one if there is a valueType - if ( parameter.phetioType && !parameter.valueType ) { - error = Validation.getValidationError( args[ i ], parameter.phetioType.validator, VALIDATE_OPTIONS_FALSE ); - error !== null && errors.push( error ); - } - } - return errors; + return this.parameters.map( ( parameter, index ) => { + return Validation.getValidationError( args[ index ], parameter, VALIDATE_OPTIONS_FALSE ); + } ); } /** Index: main/axon/js/Emitter.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/axon/js/Emitter.ts b/main/axon/js/Emitter.ts --- a/main/axon/js/Emitter.ts (revision 33fc6a74a6f8018d762f10ac580cf211d352809a) +++ b/main/axon/js/Emitter.ts (date 1678288504046) @@ -19,6 +19,9 @@ import TinyEmitter from './TinyEmitter.js'; import Tandem from '../../tandem/js/Tandem.js'; import TEmitter, { TEmitterListener, TEmitterParameter } from './TEmitter.js'; +import NullableIO from '../../tandem/js/types/NullableIO.js'; +import StringIO from '../../tandem/js/types/StringIO.js'; +import ArrayIO from '../../tandem/js/types/ArrayIO.js'; // By default, Emitters are not stateful const PHET_IO_STATE_DEFAULT = false; @@ -170,16 +173,18 @@ // Match `Emitter.emit`'s dynamic number of arguments implementation: function( this: Emitter, ...values: unknown[] ) { - const errors = this.getValidationErrors( ...values ); - if ( errors.length > 0 ) { - throw new Error( `Validation errors: ${errors.join( ', ' )}` ); - } - else { - this.emit( values ); - } + this.emit( values ); }, documentation: 'Emits a single event to all listeners.', invocableForReadOnlyElements: false + }, + getValidationErrors: { + returnType: ArrayIO( NullableIO( StringIO ) ), + parameterTypes: parameterTypes, + implementation: function( this: Emitter, ...values: unknown[] ) { + return this.getValidationErrors( values ); + }, + documentation: 'Checks to see if the proposed values are valid. Returns an array of length N where each element is an error (string) or null if the value is valid.' } } } ) ); Index: main/tandem/js/PhetioActionTests.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/tandem/js/PhetioActionTests.ts b/main/tandem/js/PhetioActionTests.ts --- a/main/tandem/js/PhetioActionTests.ts (revision cba9a2792976fb7c6d44f3af71af56ae62129d52) +++ b/main/tandem/js/PhetioActionTests.ts (date 1678289070512) @@ -64,6 +64,11 @@ tandem: Tandem.ROOT_TEST.createTandem( 'phetioAction' ) } ); + // @ts-expect-error INTENTIONAL for testing + const v = phetioAction.getValidationErrors( 'hello' ); + assert.ok( v.length === 1, 'should have one validation error' ); + assert.ok( typeof v[ 0 ] === 'string', 'should have correct validation error' ); + phetioAction.executedEmitter.addListener( ( currentCount: number ) => { assert.ok( !actionDisposedItself(), 'should not be disposed until after emitting ' + currentCount ); assert.ok( count === 3, 'count will always be last because all execute calls come before all emitting ' + currentCount ); Index: main/tandem/js/PhetioAction.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/tandem/js/PhetioAction.ts b/main/tandem/js/PhetioAction.ts --- a/main/tandem/js/PhetioAction.ts (revision cba9a2792976fb7c6d44f3af71af56ae62129d52) +++ b/main/tandem/js/PhetioAction.ts (date 1678288574553) @@ -23,6 +23,9 @@ import Emitter from '../../axon/js/Emitter.js'; import PhetioObject from './PhetioObject.js'; import StrictOmit from '../../phet-core/js/types/StrictOmit.js'; +import ArrayIO from './types/ArrayIO.js'; +import NullableIO from './types/NullableIO.js'; +import StringIO from './types/StringIO.js'; const EMPTY_ARRAY: Parameter[] = []; @@ -72,17 +75,18 @@ returnType: VoidIO, parameterTypes: parameterTypes, implementation: function( this: PhetioAction, ...values: unknown[] ) { - const errors = this.getValidationErrors( ...values ); - - if ( errors.length > 0 ) { - throw new Error( `Validation errors: ${errors.join( ', ' )}` ); - } - else { - this.execute( values ); - } + this.execute( values ); }, documentation: 'Executes the function the PhetioAction is wrapping.', invocableForReadOnlyElements: false + }, + getValidationErrors: { + returnType: ArrayIO( NullableIO( StringIO ) ), + parameterTypes: parameterTypes, + implementation: function( this: Emitter, ...values: unknown[] ) { + return this.getValidationErrors( values ); + }, + documentation: 'Checks to see if the proposed values are valid. Returns an array of length N where each element is an error (string) or null if the value is valid.' } } } ) ); ```

Can you please review + test this? If it seems good, please commit or reassign to me to do so.

[zepumph]

This worked great!

But now I'm worried about some other spots I'm seeing some weirdness in how numbers are coming across to to the sim frame (and arrays):

I'll need to keep looking into your patch, but it seems to be on the right direction.

[zepumph]

I believe this second worry was about https://github.com/phetsims/phet-io/issues/1926. @samreid can you please apply the patch and let me know how it goes? Should we MR this so that all current PhET-iO sims have getValidationErrors for Emitter and PhetioAction? I don't think it is too important, but also may not be very hard.

[samreid]

The patch in https://github.com/phetsims/axon/issues/419#issuecomment-1460330399 is too ancient to reapply cleanly. I'm happy to rewrite the patch (shouldn't take long), but I just wanted to check in beforehand. Should I recreate the patch, review/commit? I don't think there is high value in MRing this eagerly.

[zepumph]

@samreid and I made good progress here. We confirmed that https://github.com/phetsims/phet-io/issues/1926 fixed much of the trouble in https://github.com/phetsims/axon/issues/419#issuecomment-1462595493, and that we are ready change the API to remove validation errors from setValue/emit/execute calls and into a getValidationError(s)() function. It is up to you if you want to check the validity of your values.

We are happy with our commits, and found one extra case where we did in fact want that extra logic for checking (instead of having a try catch handling it in the first place.

[zepumph]

Not closing so I can check in on CT later.

[zepumph]

All looks good here! Closing