Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add worker api #36

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/umap-worker-js.js

Large diffs are not rendered by default.

15 changes: 11 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,24 +14,31 @@
"types": "dist/index.d.ts",
"scripts": {
"test": "jest",
"bundle": "rm -rf lib && webpack --config ./webpack/lib.config.ts && webpack --config ./webpack/lib.min.config.ts",
"build": "rm -rf dist && tsc && yarn bundle"
"bundle": "rm -rf lib && webpack --config ./webpack/lib.config.ts && webpack --config ./webpack/lib.min.config.ts && webpack --config ./webpack/lib.worker.config.ts",
"build": "rm -rf dist && tsc && yarn bundle",
"preyalc": "yarn build",
"build-local": "yalc publish --push",
"build-local-watch": "nodemon -w src/ -w worker/ -e js,ts -x 'yarn run build-local'"
},
"devDependencies": {
"@types/jest": "^24.0.13",
"@types/node": "^10.12.21",
"jest": "^24.8.0",
"nodemon": "^2.0.2",
"prando": "^5.1.0",
"prettier": "^1.16.4",
"ts-jest": "^24.0.2",
"ts-loader": "^5.3.3",
"ts-node": "^8.0.2",
"typescript": "^3.2.4",
"webpack": "^4.29.5",
"webpack-cli": "^3.2.3"
"webpack-cli": "^3.2.3",
"worker-loader": "^2.0.0",
"yalc": "^1.0.0-pre.35"
},
"dependencies": {
"ml-levenberg-marquardt": "^1.0.3"
"ml-levenberg-marquardt": "^1.0.3",
"uuid": "^7.0.1"
},
"publishConfig": {
"registry": "https://wombat-dressing-room.appspot.com"
Expand Down
37 changes: 37 additions & 0 deletions webpack/lib.worker.config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import * as path from 'path';

export default {
mode: 'production',
module: {
rules: [
{
test: /(\.ts$|\.js$)/,
exclude: /node_modules/,
use: ['ts-loader'],
},
{
test: /(\.worker\.ts$|\.worker\.js$)/,
exclude: /node_modules/,
use: [
{
loader: 'worker-loader',
options: { fallback: false, inline: true },
},
'ts-loader',
],
},
],
},
resolve: {
extensions: ['.ts', '.js'],
},
entry: {
lib: path.resolve(__dirname, '../worker/worker-lib.ts'),
},
output: {
filename: 'umap-worker-js.js',
libraryTarget: 'commonjs2',
path: path.resolve(__dirname, '../lib'),
},
optimization: { minimize: true },
};
4 changes: 4 additions & 0 deletions worker/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"extends": "../tsconfig.json",
"include": ["./*.ts"]
}
25 changes: 25 additions & 0 deletions worker/umap.worker.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { UMAP } from '../src/lib';

async function messageHandler({ data: { id, method, args } }) {
console.log(
`UMAPWorker: received instruction to execute 'umap.${method}' with ${args.length} args`
);
switch (method) {
case 'fit': {
const umap = new UMAP();
const embedding = await umap.fit(args[0]);
// @ts-ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeScript is incorrectly seeing the postMessages here as window.postMessages instead of Worker.postMessages and thus wanting a targetOrigin 2nd argument. This is almost certainly related to the @ts-ignore required to used the worker-loader on the module (see comment below). Getting the Worker to bundle via Webpack with TypeScript was the hardest part of this. I've bundled Workers into libs many times with the worker-loader and not had any problems in plain JS but Googling around confirms that this is indeed a common issue. (I'm also not sure how cross-browser friendly this bundling approach will be, either, since I'm not sure it works without { fallback: false, inline: true } and that may exclude some browsers...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I had to deal with this once before and I wound up opting just to redefine a type for postMessage on the window object.... kind of equally hacky but at least you get type hints on that method. As far as the browser compatibility goes I think we can cross that bridge when we get there.

postMessage({ id, method, results: { embedding } });
break;
}
default: {
// @ts-ignore
postMessage({
id,
error: `Unknown method requested of UMAPWorker: 'umap.${method}'`,
});
}
}
}

onmessage = messageHandler;
59 changes: 59 additions & 0 deletions worker/worker-lib.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import { v4 as uuid } from 'uuid';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not at all wedded to pulling in this dep to keep track of promises, just part of the proof-of-concept nature of what's currently in this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally, I'd prefer to keep this lib as dependency-free as possible (mainly for google-internal compatibility reasons) - I usually just use a start-at-one-and-increment id generating system for stuff like this.


// @ts-ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried many variations on worker-loader's advice for using it with TypeScript, but it never bundled correctly (didn't fail out during build, but the bundle would error in my example in the browser). Only ignoring all advice and using the config I've used for other projects with this @ts-ignore got it working...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is this issue open on the worker-loader, though that problem isn't exactly what I was seeing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm.... sounds awful and I'm not at all opposed to just circumventing TS if it winds up causing too much trouble.

import UMAPWorker from './umap.worker';

const requests = {};

function onError(id, error) {
console.error(error);
try {
requests[id].reject(error.message);
} finally {
delete requests[id];
}
}

function onMessage({ data: { error, id, method, results } }) {
if (error) {
onError(id, new Error(error));
}
switch (method) {
case 'fit': {
const { embedding } = results;
if (requests[id]) {
requests[id].resolve(embedding);
} else {
requests[id].reject(
`Could not find existing UMAPWorker request with id ${id}`
);
}
delete requests[id];
break;
}
default: {
throw new Error(
`Unknown method performed by UMAPWorker: 'umap.${method}'`
);
}
}
}

const worker = new UMAPWorker();
Copy link
Contributor Author

@jebeck jebeck Mar 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a worker instance should be in the constructor of the UMAPWorkerWrapper but I was failing to type it correctly (probably related to the @ts-ignore to get the import working) and moved it out to move forward with the proof-of-concept 🙃

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

worker.onerror = onError;
worker.onmessage = onMessage;

class UMAPWorkerWrapper {
fit(data) {
const id = uuid();
worker.postMessage({ id, method: 'fit', args: [data] });

const promise = new Promise((resolve, reject) => {
requests[id] = { resolve, reject };
});

return promise;
}
}

export { UMAPWorkerWrapper as UMAP };
Loading