prisma-client-py icon indicating copy to clipboard operation
prisma-client-py copied to clipboard

refactor(cli): remove pkg in favour of automatically downloading Node

Open RobertCraigie opened this issue 2 years ago • 3 comments

Change Summary

This PR completely refactors how the Prisma CLI is downloaded / installed / called. We now download Node itself at runtime and use that to install the Prisma CLI and then run it directly with Node as well.

This has some significant advantages:

  • We are now no longer tied to releases of the packaged CLI
    • These were being released by Luca, who created the Go Client and is no longer at Prisma, on his own free time.
    • Only major versions were released which means the CLI couldn't be easily tested with the latest changes on the https://github.com/prisma/prisma repository
  • Prisma Studio can now be launched from the CLI
  • The TypeScript Client can now be generated from our CLI wrapper
  • We now longer have to manually download the engine binaries ourselves, that's handled transparently for us
  • A packaged version of Node no longer has to be installed for each new Prisma version

However, this does not come without some concerns:

  • Size increase
    • We use https://github.com/ekalinin/nodeenv to download Node at runtime if it isn't already installed. This downloads and creates extra files that are not strictly necessary for our use case. This results in an increase from ~140MB -> ~300MB. -
    • However this size increase can be reduced by installing nodejs-bin which you can do by providing the node extra, e.g. pip install prisma[node]. This brings the total download size to be very similar to the packaged CLI.

How does it work?

We now resolve a Node binary using this flow:

  • Check if Node is installed globally
  • Check if nodejs-bin is installed
  • Downloads Node using https://github.com/ekalinin/nodeenv to a configurable location which defaults to ~/.cache/prisma-nodeenv

The first two steps in this flow can be skipped if you so desire through your pyproject.toml file or using environment variables. For example:

[tool.prisma]
# skip global node check
use_global_node = false

# skip nodejs-bin check
use_nodejs_bin = false

# change nodeenv installation directory
nodeenv_cache_dir = '~/.foo/nodeenv'

Or using the environment variables, PRISMA_USE_GLOBAL_NODE, PRISMA_USE_NODEJS_BIN and PRISMA_NODEENV_CACHE_DIR respectively.

The Prisma CLI is then installed directly from npm and ran directly using the resolved Node binary.

closes #370 closes #236 closes #22 closes #413

Checklist

  • [ ] Unit tests for the changes exist
  • [ ] Tests pass without significant drop in coverage
  • [ ] Documentation reflects changes where applicable
  • [ ] Test snapshots have been updated if applicable

TODO:

  • [ ] Actually resolve the current platform name
  • [ ] Write node tests
  • [ ] Improve query engine utils interface
  • [ ] Fix windows
  • [ ] Documentation
    • [ ] Document config
  • [ ] Fix TODOs in _node.py
  • [x] Fix incorrect caching location in tests
  • [x] Fix prisma_version not reading from environment variables

Agreement

By submitting this pull request, I confirm that you can use, modify, copy and redistribute this contribution, under the terms of your choice.

RobertCraigie avatar Aug 01 '22 01:08 RobertCraigie

Codecov Report

Base: 86.52% // Head: 85.99% // Decreases project coverage by -0.52% :warning:

Coverage data is based on head (d8c5ab6) compared to base (edc9b66). Patch coverage: 87.93% of modified lines in pull request are covered.

:exclamation: Current head d8c5ab6 differs from pull request most recent head 74c6d14. Consider uploading reports for the commit 74c6d14 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #454      +/-   ##
==========================================
- Coverage   86.52%   85.99%   -0.53%     
==========================================
  Files         128      124       -4     
  Lines        6240     6363     +123     
  Branches     1027     1180     +153     
==========================================
+ Hits         5399     5472      +73     
- Misses        796      827      +31     
- Partials       45       64      +19     
Impacted Files Coverage Δ
databases/main.py 0.00% <0.00%> (ø)
src/prisma/binaries/__init__.py 100.00% <ø> (ø)
src/prisma/cli/commands/version.py 95.23% <ø> (ø)
src/prisma/engine/errors.py 90.00% <ø> (ø)
src/prisma/generator/generator.py 91.27% <ø> (ø)
tests/test_cli/test_cli.py 100.00% <ø> (ø)
src/prisma/cli/commands/fetch.py 55.55% <33.33%> (-44.45%) :arrow_down:
src/prisma/_proxy.py 91.30% <66.66%> (+0.82%) :arrow_up:
src/prisma/utils.py 83.07% <66.66%> (-0.80%) :arrow_down:
src/prisma/cli/prisma.py 86.84% <80.00%> (-7.45%) :arrow_down:
... and 18 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Aug 01 '22 01:08 codecov[bot]

Seems like a good idea. The biggest advantage I think is to save the development of the CLI part. I had this in mind when developing Prisma ORM for Dart. However, adding the Node environment in the current development environment seems to be redundant and disgusting.

medz avatar Sep 08 '22 11:09 medz

😊Wait for you to add this function experimentally first, if the user feedback is not disgusting to download the Node environment, I think Prisma client development in other languages can be used for reference.

medz avatar Sep 08 '22 11:09 medz

However, adding the Node environment in the current development environment seems to be redundant and disgusting.

@medz Are you referring to the fact that Node will be installed into the user's environment? If so, that is not the case, Node will be downloaded to a temporary / cache directory that is not included in the user's PATH or environment setup in any way.

Prisma ORM for Dart

Wow! I'm excited to hear that Prisma has been ported to yet another language! Well done and I hope it goes well :)

Let me know if you run into any issues with your integration, I'm more than happy to help!

RobertCraigie avatar Sep 24 '22 19:09 RobertCraigie

I think this makes a lot of sense and KISS in terms of keeping track with the upstream project more easily which is pretty NodeJS-centric.

Question: why not make the prisma[node] the default to quell the size increase concerns?

jacobdr avatar Oct 16 '22 19:10 jacobdr

Something somewhere is choking on the installation via .[node] expecting npm and node on the PATH. Need to track that down a bit more

[email protected] /home/prisma/.local/lib/python3.10/site-packages/nodejs/lib/node_modules/npm
prisma@c7ca946a4a90:~/.local/lib/python3.10/site-packages/nodejs$ /home/prisma/.local/lib/python3.10/site-packages/nodejs/bin/node /home/prisma/.local/lib/python3.10/site-packages/nodejs/lib/node_modules/npm/bin/npm-cli.js install [email protected]
npm notice
npm notice New minor version of npm available! 8.12.1 -> 8.19.2
npm notice Changelog: https://github.com/npm/cli/releases/tag/v8.19.2
npm notice Run npm install -g [email protected] to update!
npm notice
npm ERR! code 127
npm ERR! path /home/prisma/.local/lib/python3.10/site-packages/nodejs/node_modules/prisma
npm ERR! command failed
npm ERR! command sh -c node scripts/preinstall-entry.js
npm ERR! sh: 1: node: not found

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/prisma/.npm/_logs/2022-10-16T20_57_02_337Z-debug-0.log

jacobdr avatar Oct 16 '22 20:10 jacobdr

Update to the above -- its coming from the @prisma/engines somewhere, but still need to track that down even more:

~/.local/lib/python3.10/site-packages/nodejs$ /home/prisma/.local/lib/python3.10/site-packages/nodejs/bin/node /home/prisma/.local/lib/python3.10/site-packages/nodejs/lib/node_modules/npm/bin/npm-cli.js install @prisma/[email protected]

npm ERR! code 127
npm ERR! path /home/prisma/.local/lib/python3.10/site-packages/nodejs/node_modules/@prisma/engines
npm ERR! command failed
npm ERR! command sh -c node scripts/postinstall.js
npm ERR! sh: 1: node: not found

jacobdr avatar Oct 16 '22 21:10 jacobdr

Actually, I think its really coming from the @prisma/client dependency:

DEBUG=* python -m nodejs.npm install --foreground-scripts @prisma/client

sh: 1: node: not found
npm ERR! code 127
npm ERR! path /home/prisma/node_modules/@prisma/client
npm ERR! command failed
npm ERR! command sh -c node scripts/postinstall.js

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/prisma/.npm/_logs/2022-10-16T21_18_07_368Z-debug-0.log

https://github.com/prisma/prisma/blob/5924663527dd06c2366c2cc850875fe5c5d6774e/packages/client/scripts/postinstall.js

jacobdr avatar Oct 16 '22 21:10 jacobdr

Question: why not make the prisma[node] the default to quell the size increase concerns?

The only reason I wouldn't want to do that is because making it optional means that on machines that already have node we will be redundantly downloading Node again when we could just use the already installed Node.

Something somewhere is choking on the installation via .[node] expecting npm and node on the PATH. Need to track that down a bit more

Oh this is an interesting bug, thank you for finding it before this was merged!

RobertCraigie avatar Oct 17 '22 19:10 RobertCraigie

Fair enough re: your point above but if someone does not have NodeJS a first-time user is going to have a worse experience with a pip install. Also, anyone who is currently using this in CI is likely to experience this as a breaking change unless they updated their own dependency on us to specify the [node] extra. I don't think most people building in Python land will have an installed node on their system.

If anything I think it might be safer to download a controlled NodeJS version ourselves and invert the logic -- NOT installing node if that is specified as an extra. Not sure if that is even really possible in the Python packaging ecosystem but I guess we would look into it.

Granted, everything is pre 1.0 right now but given the popularity you have already I think we would not want to squander it with a breaking change.

jacobdr avatar Oct 17 '22 20:10 jacobdr

@RobertCraigie I know you still have some TOODs but thought I would leave an early review. Let me know when you think this is ready for me to check out and run locally. We're going to have to test the shit out of this PR

jacobdr avatar Oct 18 '22 16:10 jacobdr

Also, anyone who is currently using this in CI is likely to experience this as a breaking change unless they updated their own dependency on us to specify the [node] extra.

This shouldn't be a breaking change for anyone. I think you might've thought it would be as I actually forgot to include nodeenv as a base dependency in this PR which I hadn't realised until now.

If anything I think it might be safer to download a controlled NodeJS version ourselves and invert the logic -- NOT installing node if that is specified as an extra. Not sure if that is even really possible in the Python packaging ecosystem but I guess we would look into it.

Yeah I gavelled about this while working on this PR initially - it is not possible to have "default" extras which means we wouldn't be able to provide an option to not install Node even if it is already installed.


I think a good solution to this problem is to document the node extra as much as possible, e.g. wherever we document how to install the package, it also includes [node], that way it is very visible and hopefully most first time users would install it this way and anyone who cares about not installing Node to site-packages can just omit the node extra.

RobertCraigie avatar Oct 22 '22 18:10 RobertCraigie

Interestingly, we actually don't need the node dependency at runtime, right? Not to throw more confusion into the mix, but at "production" runtime (not dev) I believe we only need the generated Python code and the WASM engine binary. Just thinking through attack surface area, dragging in node+npm is pretty crappy (totally disregarding the size concern).

To be honest I don't quite have a solution yet to that as I need to mull it over more, but wanted to call it out (and please do correct me if I am wrong)

jacobdr avatar Nov 05 '22 16:11 jacobdr