Performance tooling: profiling harness + render-equivalence suite#2078
Open
trvrb wants to merge 23 commits into
Open
Performance tooling: profiling harness + render-equivalence suite#2078trvrb wants to merge 23 commits into
trvrb wants to merge 23 commits into
Conversation
The previous interface was slightly cumbersome - I often found myself wanting to source both datasets & narratives from some directory X and having to type in the rather long `--datasetDir X --narrativeDir X`, and often I'd misspell / pluralise the argument names; using a simple positional argument `X` is much nicer. The ability to use multiple directories is also functionality I've long wanted. The previous CLI interface is preserved in its entirety for backwards compatibility and is mutually exclusive with the new positional-args style.
Stands up the default getDataset handler on an ephemeral port and makes real HTTP requests against the JSONs in test/fetched-jsons, covering both the main (v2) dataset and the tip-frequencies sidecar. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Convert all server-side code (cli/, auspice.js, index.js, src/version.js) and the test file from CommonJS require/module.exports syntax to ES module import/export syntax. This also addresses the downstream effects on webpack bundling for the frontend code in src/. package.json: Added "type": "module" to declare the package as ESM. This makes Node treat all .js files as ES modules by default. Replaced the "main" field with "exports" to use the modern package entry point specification. Added babel-plugin-transform-import-meta as a dev dependency for Jest compatibility. Server code (cli/, auspice.js, index.js, src/version.js): Converted all require() calls to import statements and all module.exports/exports assignments to named or default exports. For packages that use CommonJS internally (argparse, webpack config), createRequire(import.meta.url) is used to obtain a require function that can load CJS modules from an ESM context. __dirname shim: Since __dirname is not available in ES modules, files that reference it (cli/utils.js, cli/build.js, cli/develop.js, cli/view.js) now derive it via path.dirname(fileURLToPath(import.meta.url)). The test file also uses this pattern. Dynamic require(handlersPath) in cli/view.js: The user-provided custom handlers path was previously loaded with require(). This is now await import(handlersPath), which required making customRouteHandlers() and run() async functions. Config file renames: webpack.config.js and babel.config.js are renamed to .cjs since they use CommonJS syntax (module.exports) and would otherwise be treated as ESM due to the package-level "type": "module" setting. Babel transform for Jest: Jest runs tests through babel-jest which transforms ESM to CJS. However, babel leaves import.meta.url untouched, causing a runtime error when Jest executes the transformed code. Added babel-plugin-transform-import-meta (test env only) to convert import.meta.url to its CJS equivalent during testing. The test/server-fetch.test.js file itself was also converted to ESM syntax. webpack.config.cjs updates: Added resolve.fullySpecified: false (both top-level and as a per-module rule) to allow extensionless imports in the bundled frontend code -- without this, webpack enforces mandatory file extensions for all imports in "type": "module" packages. The JS/TS rule also sets type: "javascript/auto" so webpack parses source files in mixed mode, recognizing both require() and import syntax. module.hot to import.meta.webpackHot: In src/root.js and src/store.ts, the webpack HMR API was accessed via module.hot which is a CJS-only global. Replaced with import.meta.webpackHot, the ESM equivalent. The HMR accept callback in store.ts now uses dynamic import() instead of require() to reload reducers. Frontend require() calls for assets: Several React components used require() to load images (SVG, PNG, JPG). These are converted to static import statements at the top of each file, which webpack asset/resource loader handles identically. Affected files: navBar/content.js, splash/splash.js, framework/fine-print.js, framework/footer-descriptions.js, info/byline.js. Dynamic extension loading (src/util/extensions.ts): The extensions.ts file retains its synchronous require(`@extensions/...`) pattern. This works because webpack's javascript/auto parser mode (set via the module rule) handles require() at build time even in a "type": "module" package. An earlier approach using top-level await with dynamic import() caused async module semantics to cascade through the dependency graph (via util/globals.js), which broke component rendering by wrapping module exports in async module namespace objects. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Convert 8 cli files from .js to .ts, leveraging Node 24's native type stripping to run them directly without a build step. tsc is used for type-checking only (noEmit). Files converted: - cli/utils.ts - cli/server/getDataset.ts - cli/server/getDatasetHelpers.ts - cli/server/getAvailable.ts - cli/server/getNarrative.ts - cli/server/processPaths.ts - cli/view.ts - cli/develop.ts - cli/build.ts Supporting changes: - Add cli/tsconfig.json extending root with nodenext module resolution and allowImportingTsExtensions for Node-native .ts imports - Add allowImportingTsExtensions to root tsconfig.json (needed because cli .ts files are pulled in transitively via test imports) - Exclude cli/**/* from root tsconfig include/add to exclude (prevents checking cli files under the wrong moduleResolution) - Update package.json type-check script to run both tsconfigs - Update all import paths across 11 files (.js -> .ts extensions) - Update webpack.config.cjs require for cli/utils.ts - Add eslint-disable for explicit-function-return-type and consistent-type-assertions in converted files (incremental migration) - Apply chalk 2.x workaround (type assertion) for nodenext compat - Fix TypeScript strict errors: unknown in catch blocks, Object.entries casting for Set methods, undefined-in-object check Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The docs and code had fallen out of sync over the years.
* `parseNarrativeFile` was not only deprecated as the docs indicated
("This function is deprecated as of vXXX"), it no longer existed in the codebase.
* The docs for the getNarrative route incorrectly indicated the `type` query as optional
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Triggered by existing warnings: WARNING: You are currently running a version of TypeScript which is not officially supported by @typescript-eslint/typescript-estree.
Errors were new since version bump in parent commit
This is an old and now unused test that's been fully superseded by the Playwright smoke tests. It was the only test that used "chai" (and "mocha", although that wasn't listed as a dep). I only noticed this as the eslint version bump flagged up an error in the test file
This functionality never worked as well as hoped. Bigger picture, this represents a move away from pre-provisioned datasets / narratives and towards "bring your own data", which is the main use case after all!
(This was from an ancient method of deploying auspice)
to be compatible in a world without the main 'data/' directory. This made me realise our "smoke-tests" are basically a single test! Nonetheless it's worth keeping around the architecture for future improvements
This is a relic of earlier times where it was kept in-sync with the canonical nextstrain datasets. We maintain similar functionality in the fetch-test-data script but for testing purposes only. This is a key part of the bigger re-design which moves auspice to a visualisation program/library which you point at your own data.
It has previously been useful to run Auspice as a stand-alone Heroku (review-) app, but this has been superseded by standing up a testing nextstrain.org review app as the range of datasets/narratives available is much greater, or a auspice.us app for datasets not available via nextstrain.org. It's simpler (and cheaper!) to drop the Auspice app entirely
Slightly out of date!!!
[v3] [cli] Allow one or more positional path args
[v3] Various JS/CS CJS/ESM updates
[v3] Default data changes
Instrument the incremental SVG-update path so the profiling harness can attribute
it: wrap modifySVG in a timer, and add the missing timerEnd("mapToScreen") (its
timerStart had no matching end, which tripped a race-condition warning and never
logged). Timer calls are stripped from normal builds; they are compiled in only
via `auspice build --includeTiming`.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
A reusable, measure-only harness (test/profiling/, `npm run profile`) that builds a --includeTiming production bundle, serves the local data/ dir, drives Auspice headless against representative datasets, captures the perf.js console timers, and writes a ranked baseline (with before/after diffing via --baseline). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
renderEquivalence.mjs + domSnapshot.mjs assert that the incremental tree-update path (phylotree.change()) produces DOM identical to a from-scratch full render of the same state, for each operation and for sequences of operations. Driving needs no source changes: history.pushState + a popstate event triggers the app's own listener, which takes the incremental URL_QUERY_CHANGE branch — the same change() path a control click uses. The golden reference is a fresh page at the app's resulting URL. The comparator snapshots per-id tips/branches/conf (reading style+attr, normalizing units/precision) and reports per-element diffs. Covers colorBy (incl. genotype/continuous), all layouts, distance, date + trait filters, zoom, temporal confidence, and combinations, across ebola/zika/spike-sm. Run: `npm run render-equiv`. Supersedes the one-off verifyRenderEquivalence check. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
trvrb
changed the title
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Via Claude
Summary
Tooling for the performance effort (no user-facing behavior change). The actual render optimizations land in a stacked follow-up PR (
perf-implement→ this branch), so the tooling can be reviewed separately from the behavioral changes.Three commits:
Profiling timer instrumentation — wraps
modifySVGin atimerStart/Endand adds a missingtimerEnd("mapToScreen")(itstimerStarthad no matching end, tripping a race warning and never logging). Timer calls are stripped from normal builds — compiled in only viaauspice build --includeTiming— so this is inert in production.Headless profiling harness (
npm run profile,test/profiling/) — builds a--includeTimingproduction bundle, serves the localdata/dir, drives Auspice headless against representative datasets, captures theperf.jsconsole timers, and writes a ranked baseline with before/after diffing (--baseline).Render-equivalence regression suite (
npm run render-equiv) — for each operation (colorBy, layout, distance, filter, zoom, confidence, …) and for sequences of operations, drives the incrementalphylotree.change()path (viahistory.pushState+ apopstateevent — the app's own listener, zero source changes) and asserts the settled SVG DOM is identical to a from-scratch full render of the same end state. A mismatch is a stale-DOM regression. Covers ebola/zika/spike-sm.Test plan
npm run render-equiv -- --build→ all scenarios green (blanket-flag behaviour is trivially equivalent on this branch).npm run profile -- --build→ produces a ranked baseline.tsc+eslintclean.🤖 Generated with Claude Code