Open samreid opened 1 year ago
Yes, it is better for checking out SHAs at a certain date to not have breaks committed to master.
I used that feature this week to search for the break in https://github.com/phetsims/chipper/issues/1323#issuecomment-1295494831. Only one of the dozen samples was broken, which was great and made it easy to trace the problem.
I am going to try feature branches without commit hooks when working in a single repo for a bit!
Pros: OK to commit unstable/broken code quickly without using commit hooks. People checking out master won't get that unstable code. Cons: Potential for merge conflicts, others could be committing to master at the same time. Code won't be tested on CT (unless we add facets for that).
Some strategies, brainstorming and paper trail from discussions with @marlitas and @zepumph yesterday:
OK to commit broken code locally, then use a pre-push hook to make sure everything is consistent before pushing. How would that help people that basically push every commit? Would we change our workflow? What about checking out broken code by date or bisect?
Commit broken code locally, then squash (bad?) commits before push? But then you lose granularity and have to make up new commit messages.
Persist CT values and use that as a guide when git bisecting Why should we have to check with CT? Why can't everything always be green? Because it takes 30+ minutes to run all tests locally.
How can we speed up tests??? Sims launch faster. Run everything in parallel. Subset of phet-io sims.
phet-io API tests only took 25% on Michael's machine. It is not the bottleneck, especially if it runs in parallel with other things.
Would restoring project references help at all?
Other TypeScript type checking performance improvement ideas: https://github.com/microsoft/TypeScript/wiki/Performance
Should we run lints in different processes?
Can we just check in with WebStorm and ask "hey webstorm, are there any type errors or lint errors?" There is a Before Commit "analyze code" inspection?
For sims, only type check sims.
You can profile the type checker to see where the bottlenecks are.
Should we transition from precommit hooks to prepush hooks? Or can we speed up the precommit hooks? https://github.com/phetsims/chipper/issues/1269
Type check downstream https://github.com/phetsims/perennial/issues/272
Type checking takes too long to be a precommit hook: https://github.com/phetsims/chipper/issues/1160
Precommit hooks should check in with watch process https://github.com/phetsims/chipper/issues/1174
We did project references in https://github.com/phetsims/chipper/issues/1055
Table with timing is https://github.com/phetsims/chipper/issues/1241
I switched to Promise.all, and tested with echo $?
, and with early failing, and error messages. Everything seems to be working OK except for the nesting of the phet-timing-log, which now looks like this:
<!-- 11/9/2022, 4:49:08 PM -->
<hook-pre-commit repo="chipper">
<lint>
<report-media>
<tsc>
<qunit>
<phet-io-api-compare>
</qunit> <!-- 310ms -->
</report-media> <!-- 335ms -->
</tsc> <!-- 416ms -->
</hook-pre-commit repo="chipper"> <!-- 1206ms -->
OK I also fixed the depth. Looks like this now:
<!-- 11/9/2022, 5:06:46 PM -->
<hook-pre-commit repo="chipper">
<lint>
<report-media>
<tsc>
<qunit>
<phet-io-api-compare>
</lint> <!-- 378ms -->
</phet-io-api-compare> <!-- 375ms -->
</report-media> <!-- 394ms -->
</qunit> <!-- 399ms -->
</tsc> <!-- 468ms -->
</hook-pre-commit repo="chipper"> <!-- 473ms -->
This part of the issue is ready for review.
Maybe for next steps, we could gather a few days of data for how long things are taking now, and focus on the bottleneck task?
Slack#developer:
Chris Malley Today at 12:44 AM I noticed that there were some changes to git hooks today. When I encounter errors, I’m now seeing duplicate errors in WebStorm console, for example see below. Is it possible that something is being run twice?
0 errors in 16559ms /Users/cmalley/PhET/GitHub/natural-selection/js/common/view/NaturalSelectionTimeControlNode.ts 14:8 error 'merge' is defined but never used @typescript-eslint/no-unused-vars 17:8 error 'AssertUtils' is defined but never used @typescript-eslint/no-unused-vars 21:8 error 'Tandem' is defined but never used @typescript-eslint/no-unused-vars ✖ 3 problems (3 errors, 0 warnings) Task failed: lint, /Users/cmalley/PhET/GitHub/natural-selection/js/common/view/NaturalSelectionTimeControlNode.ts 14:8 error 'merge' is defined but never used @typescript-eslint/no-unused-vars 17:8 error 'AssertUtils' is defined but never used @typescript-eslint/no-unused-vars 21:8 error 'Tandem' is defined but never used @typescript-eslint/no-unused-vars ✖ 3 problems (3 errors, 0 warnings)
Michael Kauzmann :spiral_calendar_pad: 1 hour ago Most likely it's just duplicate logging. "lint" is in a child process now, so there are probably two spots where we report logging from the subtask. I'd just note it in https://github.com/phetsims/chipper/issues/1350
Not sure what else to do about this issue at the moment, @zepumph or @AgustinVallejo do you recommend further changes? Want to meet again? Check in with dev team?
No comments from me! I think it's ready for dev team
I think it would be awesome to meet again together in the next week or two, and perhaps work on another part of the speed up potential. Running things in parallel is working well for me so far, but I haven't made many commits in the last week, so I would like more data still.
Can we please get the duplicate error messages fixed, reported 12 days ago in https://github.com/phetsims/chipper/issues/1350#issuecomment-1310608107? It's not only annoying, but it's actually making me miss errors.
EDIT: Move to https://github.com/phetsims/chipper/issues/1359
I added a calendar meeting for 12/9 for 1 hour. Let's touch base then.
From above: How can we speed up tests???
general improvement
phet-io-api compare test:
tsc
We worked a bit more on project references https://github.com/phetsims/chipper/issues/1356 and are still figuring out how to handle mixins.
@marlitas said #1325 relates to this. I added assignees for this subgroup. @zepumph said it would be good to schedule regular meetings on this topic until it is better under control.
I just had a particularly slow experience while using pre-commit hooks with PhET-iO API validation:
1) Run pre-commit hooks on change in scenery-phet (~3 minutes )
2) Hooks identified a PhET-iO API break, so I regenerated PhET-iO API (~1.5 minutes)
3) Re-run pre-commit hooks (2 minutes)
4) Commit changes (0.5 minutes)
5) Ready to push! ...push failure -> merge required -> pull scenery-phet with --rebase (0 minutes)
6) Run pre-commit hooks...failure because the scenery-phet change is incompatible with my axon (~2 minutes)
7) Stash my changes, pull all, re-apply my changes, restart transpiler (0.5 min)
8) Re-generate PhET-iO API (1.5 minutes)
9) Run pre-commit hooks (~2 minutes)
10) commit (0.5 minutes)
11) Successful push
Id be interested in looking joining this sub-group issue or looking into workflow alternatives.
This will need time carved out to work on it. I have recommended it for iteration. Unassigning.
For sim code I have been working in feature branches mostly for the past few weeks and it has improved my productivity. In https://github.com/phetsims/perennial/issues/276 we made it so pre-commit hooks only run for master. Removing my assignment until we create dedicated time for this.
You can use node chipper/js/scripts/precommit-hook-multi.js
to check all precommit hooks on the repos with changes.
In WebStorm/Intellij, I have the following external tool defined that lets me in-parallel lint/tsc. It runs pretty quickly now, and will link any failed lines.
Setup here:
The above commits by @zepumph have a markdown formatting problem, reported in https://github.com/phetsims/aqua/issues/210.
Today we had another dev meeting discussion on phet-io api checking in pre commit hooks. Here is a copy of the discussion:
Basically, it seems like we may want to allow git hooks to be more flexible, and rely on running more checks manually before commit, or face the wrath of CTQ.
From https://github.com/phetsims/chipper/issues/1342#issuecomment-1293907234 which came from https://github.com/phetsims/chipper/issues/1325, we would like to improve the precommit hook process.
@zepumph: How should we use this tool? I often have commit hooks that take longer than 100 seconds. What is an acceptable amount of time? @zepumph - sometimes to get around commit hook timing I commit locally without hooks and then run tests before pushing. @marlitas - I thought we shouldn't be making breaking changes even locally? @jonathanolson - Yes, it is better for checking out SHAs at a certain date to not have breaks committed to master. @jonathanolson - Have the commit hooks been successfully saving us from pushing breaking changes? @pixelzoom @jbphet @jessegreenberg - Yes! @marlitas - I think it would be nice to be able to make rapid commits locally or to a branch without waiting for commit hooks. @jbphet - You could make commits to a feature branch. Then when merge to master when the changes are complete. @samreid - Lets form a sub team for this. We have log data to look through now, and need a sub team to make decisions about this. @zepumph - I waited for built tools for 18.5 minutes yesterday. The time could be longer. Sometimes run without commit hooks. @jessegreenberg - I am going to try feature branches without commit hooks when working in a single repo for a bit! @kathy-phet - After the POSE convening we can have a sub-group discuss this.