Add support for PhET-iO API breaking and design change tracking

[samreid]

From https://github.com/phetsims/phet-io/issues/1750, we would like to add PhET-iO API breaking and design change tracking to sun. One of the steps will be instrumenting the demo and some of its components that we want to track.

I reached out to @jonathanolson and @pixelzoom about how DemoScreenView could be adapted to support this, here is our conversation from slack:

Sam Reid 3:44 PM Hi Chris & Jonathan, Michael & I would like to adapt the sun demo to add phet-io support for API tracking for sun components for https://github.com/phetsims/phet-io/issues/1750#issuecomment-812221438. One of the steps to make this work would be to eagerly create any phet-io components (instead of lazily as we do now). Do you think that will be OK? (edited)

Jonathan Olson 3:52 PM I'm not sure I understand the full context

Sam Reid 4:52 PM We would like to treat the sun demo as an “instrumented sim” so that we can track the API of common code components such as sliders. Then we will create several instrumented example components, such as sliders, to make sure their API doesn’t pick up a backward-incompatible change or deviate from the specified design.

Chris Malley 5:09 PM Will you want to do the same to scenery-phet demo? (edited)

Sam Reid 5:10 PM Yes and other common code repos eventually scenery-phet is already partially instrumented

Chris Malley 5:25 PM

Do you think that will be OK? I’m not clear what you’re asking. From a “good design” perspective, it feels wrong to eagerly instantiate N demos when 1 or 2 are likely to be of interest and actually get run during a “demo session”. But such is life with PhET-iO. Creating demos dynamically for PhET-iO would be a huge hassle/cost, so I don’t think you have much of a choice there. The current demos should be easy enough to convert to “eagerly”, as long as someone hasn’t written a demo that depends on being created lazily. Demo coverage of components isn’t complete, so you’re not going to get complete PhET-iO API coverage, but it will be start. Factor in the cost to create more demos to improve coverage. Demos will of course take longer to start up, and startup time will increase as demos are added.

Did I answer the question? :)

How about making the default behavior of demos be “lazy”, and add a query parameter for “eager”? Use “eager” for checking PhET-iO API (and maybe CT test?). And for daily use, we’ll still benefit from lazy instantiation. (edited)

Chris Malley 5:34 PM Should be able to handle this “eager” query parameter in DemosScreenView.js. Something like (untested):

Index: js/demo/DemosScreenView.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/demo/DemosScreenView.js b/js/demo/DemosScreenView.js
--- a/js/demo/DemosScreenView.js    (revision 9a1170b9782876dcc190e98e2aefb25bddbaf9ce)
+++ b/js/demo/DemosScreenView.js    (date 1617838415459)
@@ -43,7 +43,7 @@
       // {boolean} see https://github.com/phetsims/sun/issues/386
       // true = caches Nodes for all demos that have been selected
       // false = keeps only the Node for the selected demo in memory
-      cacheDemos: false,
+      cacheDemos: true,
       tandem: Tandem.REQUIRED
     }, options );
@@ -127,6 +127,12 @@
         demosParent.addChild( demo.node );
       }
     } );
+
+    if ( phet.chipper.queryParameters.eagerDemos ) {
+      demos.forEach( demo => {
+        selectedDemoProperty.value = demo;
+      } );
+    }
   }
 }

5:35 Or better: cacheDemos: phet.chipper.queryParameters.eagerDemos 5:37 Actually, that’s not better. After merge, do this: if ( phet.chipper.queryParameters.eagerDemos ) { cacheDemos: true }

(edited)

Sam Reid 6:04 PM

From a “good design” perspective, it feels wrong to eagerly instantiate N demos when 1 or 2 are likely to be of interest and actually get run during a “demo session”. I don’t think we need to display all of the components we instantiate for phet-io API concerns. Demos will of course take longer to start up, and startup time will increase as demos are added. I tried making everything eager in the sun demo and it still started up quickly (on my computer) (edited)

Chris Malley 6:05 PM What does displaying them have to do with it?

Sam Reid 6:05 PM I don’t know? I was wondering about your comment.

Chris Malley 6:05 PM The patch above will eagerly instantiate and cache them. 6:06 Similar to if you set cacheDemos: true and then manually selected each demo.

Sam Reid 6:06 PM I see 6:09 Can we eliminate cacheDemos (so everything is always cached) and laziness (so everything is always eager) completely? (edited)

Chris Malley 6:10 PM That’s what I said in https://phetsims.slack.com/archives/G01MJQFK8F5/p1617837933014500. But it seems lazy (and unnecessary) to eliminate laziness. 6:11 So yes, it can be done. Must it be done for PhET-iO? In this case, no.

Sam Reid 6:12 PM It seems cacheDemos is always true, do we need to keep it around for some reason? 6:15 What if (for now), we put the phet-io api components separately from the createDemo? Like so: class ButtonsScreenView extends DemosScreenView { constructor( options ) { options = merge( { tandem: Tandem.REQUIRED }, options ); super( [ { label: 'PushButtons', createNode: demoPushButtons }, { label: 'RadioButtons', createNode: demoRadioButtons }, { label: 'ToggleButtons', createNode: demoToggleButtons }, { label: 'MomentaryButtons', createNode: demoMomentaryButtons } ] ); // Not displayed this.resetAllButton = new ResetAllButton( { tandem: tandem.createTandem( 'resetAllButton' ) } ); } }

Chris Malley 6:15 PM Probably don’t need it. If we did need it, it should probably be specified per demo, in case we had some demo that was a memory hog and wanted to reclaim memory when done with it. But I realize that developers today tend to ignore resource (CPU, memory) usage until it becomes a problem. 6:16 So sure, dump it. 6:16 What if (for now), we put the phet-io api components separately from the createDemo? I don’t get it. Why? 6:17 What’s the problem with just looping through the array of demos and instantiating all of them at startup?

Sam Reid 6:17 PM I’ll eliminate cacheDemos in https://github.com/phetsims/sun/issues/692 6:18 What’s the problem with just looping through the array of demos and instantiating all of them at startup? That sounds reasonable, but I wanted to avoid the complication of having to specify different query parameters for API testing or on CT, etc.

Chris Malley 6:18 PM It seems cacheDemos is always true, do we need to keep it around for some reason? cacheDemos: false is the default

Sam Reid 6:18 PM Oops, I misread my working copy changes. Sorry (edited)

Chris Malley 6:18 PM And I don’t see any subclasses overriding that. (edited) 6:22 So you want to add an entirely different set of components that are not displayed, and solely for PhET-iO API verification? I can think of lots of problems with that. We have to create and maintain 2 sets of demo components. You won’t (visually) see problems they might have. The whole point of lazy instantiation is defeated. …. You’d be better off just instantiating the one set of demos eagerly. (edited)

Sam Reid 6:22 PM I see 3500ms startup time in scenery-phet demo lazy vs 5000ms eager. Is that too slow? (edited)

Chris Malley 6:23 PM Entirely not the point. (edited)

Sam Reid 6:24 PM Should we opt-in to eager on a component by component basis? class ButtonsScreenView extends DemosScreenView { constructor( options ) { super( [ { label: 'PushButtons', createNode: demoPushButtons, lazy: true }, { label: 'RadioButtons', createNode: demoRadioButtons, lazy: false }, { label: 'ToggleButtons', createNode: demoToggleButtons }, { label: 'MomentaryButtons', createNode: demoMomentaryButtons } ] ); } } 6:24 I can think of lots of problems with that. I agree! Still looking for best solution

Chris Malley 6:25 PM If in the future I add a demo that takes 4 seconds to instantiate, should everyone have to endure that 4 seconds so they can inspect (e.g.) the ResetAllButton?

Sam Reid 6:25 PM Nope, but devils advocating: you could run with ?screens=1&component=ResetAllButton if you need to iterate quickly

Chris Malley 6:26 PM I feel like we’re talking in circles. You have my opinion. Feel free to proceed.

Sam Reid 6:26 PM Can you copy/paste the best/most recent opinion? I’m lost

Chris Malley 6:27 PM Default to lazy instantiation, add query-parameter support for eager instantiation. Easily done in the DemosScreenView base class.

Jonathan Olson 6:27 PM I'm late to the discussion, but wouldn't we use capsules for things like this lazy approach?

Chris Malley 6:28 PM Yep. But I’d be writing a lot less demos if I have to deal with PhetioCapsule and PhetioGroup.

Jonathan Olson 6:28 PM For what it's worth, I regularly use the screens query parameter to speed up the scenery-phet demo, so the concept of all demos loading does sound like a headache that I would like to avoid, but I understand if it's more expensive to do otherwise

Sam Reid 6:29 PM Does lazy or eager need to be all-or-none? Or should components indicate if they want to be eager, then the query parameter eagers them?

Jonathan Olson 6:29 PM Why not create a repo for this, that can pull components from anywhere? 6:29 and that has its own "demo" custom made for phet-io purposes?

Sam Reid 6:30 PM Hmmm.. Interesting idea. But it seems more modular to put the Slider API declaration and tests in the Slider repo 6:31 out for a bit, thanks for your consultations!

[samreid]

I realized we can use PHET_IO_ENABLED as the flag to turn on the "create trackable API component" mode. However, there are several hassles for this. Generating the API currently requires phetioValidation to be turned on. I don't think we should add a mode where we can generate an api and opt out of validation, so we'll need to provide Tandem.OPT_OUT in a few places.

[samreid]

@zepumph and I worked on a first version of this today. Next step will be to commit the API file to phet-io/api and add sun as a stable repo.

[samreid]

Since we will use the phetioDesigned:true approach as part of https://github.com/phetsims/phet-io/issues/1758#issuecomment-828694118, we will immediately have coverage of lots of common code components from the 5 api-stable sims. In the future, we can improve on this test harness for when we have instrumented components that aren't exercised in a phetioDesigned:true sim (or exercised in a certain way in a phetioDesigned:true sim).

[samreid]

Another solution would be to have 2 entry points for sun, one for the visual demo and another for the phet-io instrumentation coverage. In a related comment, I just punted on https://github.com/phetsims/sun/issues/691, but maybe it will unblock it if we can have 2 entry points or 2 modes (one standard and one for phet-io). @zepumph what do you recommend?