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

Make nblas a peer dependency #96

Closed

Conversation

bartvanandel
Copy link
Contributor

Although 'vectorious.js' in current master treats 'nblas' as though its presence is optional, trying to use the package as an npm dependency will fail if 'nblas' cannot be installed. I assume this is undesired behavior, because it can run perfectly fine without it, and this is in fact necessary when using the library in the browser (e.g. client-side only code).

This patch addresses this issue by making nblas a peer dependency explicitly. This means that users of vectorious will have to add nblas to their dependencies themselves. It would also be possible to make nblas an optional dependency (using 'optionalDependencies' instead of 'peerDependencies'), which would cause npm to try and install nblas but continue (not fail) when installing fails.

Furthermore, two explicit entry points 'withblas' and 'withoutblas' have been added so developers can select either when required. This would for example be desired when 'nblas' is (or can be) installed locally without issues, but the developer is building for the web, i.e. without node as a backend.

The default entry point is still 'vectorious.js' and behaves just like before. The part which relies on 'nblas' has been refactored into a separate file though so it can be used by the 'withblas' easily as well.

@bartvanandel
Copy link
Contributor Author

One more thing: when I tried to run the unit tests on my machine (using 'npm run test'), the command failed. This had to do with the script being defined as './node_modules/mocha/bin/mocha'. I'm on Windows, apparently this means the command is being interpreted as '.' with arguments '/node_modules', '/mocha', '/bin' and '/mocha' again. By changing the script to simply 'mocha' this was resolved. Npm already knows how to run it as the 'package.json' file of the mocha package tells npm where it is.

@mateogianolio
Copy link
Owner

This is great but I'll wait with merging until I have decided between optional and peer dependency.

@bartvanandel
Copy link
Contributor Author

Fair enough. I tried both approaches and to me, the peer dependency seemed cleaner. Yes, it requires the user to install the dependency manually, but in case no suitable compiler is installed (which is currently required for blas) it results in less bloat when installing fails. Both options however will proceed here.

@mateogianolio
Copy link
Owner

The best solution would be if nblas would contain javascript fallbacks for all blas routines, but unfortunately that's not going to happen anytime soon. I like the idea of peer dependencies but it will break how the library currently works and that might confuse users.

@bartvanandel
Copy link
Contributor Author

Well that would be one option, although that means nblas should also work without native bindings. I agree we can't currently rely on that going to happen anytime soon.

Your point on putting the responsibility of installing nblas manually in the hands of the end user is a valid one. I didn't try both options for no reason, in fact optional dependencies were my first attempt at getting this library to work for us. I guess my main objection was the amount of errors you run into when nblas failed to install. Now if it were possible to reduce this, I'd say go for optional dependencies straight away.

Maybe we should look into other libraries using peer dependencies and see what their arguments (or downsides) are.

@bartvanandel
Copy link
Contributor Author

Since #103 was merged in favor of this one, I moved all other changes into separate PRs (#104, #105, #106) and removed the original branch for this here PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants