Remove all dependencies from core ehrql functionality #2225
Conversation
Deploying databuilder-docs with
|
| Latest commit: |
1adc3cc
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://53c280d8.databuilder.pages.dev |
| Branch Preview URL: | https://minimal-dependencies.databuilder.pages.dev |
|
This currently failing coverage because of this bug: pytest-dev/pytest-cov#667 We need to either remove --dist=loadgroup from the default, or always run with a -n param too, I think |
I think so. I don't think it should use the controller it does unless you are running with -n. Basically, its hardcoded detection of xdist being used is wrong. |
There was a problem hiding this comment.
I'm slightly hesitant about merging this to be honest, because at things stand it adds complexity without giving us any direct benefit. It's good to know how we'd do this kind of optional import if we needed to, but we don't need to right now.
I'm also a bit surprised that pyarrow is the only troublesome dependency here. I thought we end up importing the SQLite engine inside ehrql.main which ends up dragging in sqlean? But if you've tested that this does all work in Pyodide with just pyarrow removed then I'm prepared to believe it.
Understood. When this work was done, we were still exploring running it in pyodide, but that has been punted in favour of vscode extension, AIUI.
It's not, I have follow on branch that defers imports for some commands (sqlalchemy et al for dump_dataset_sql and graphviz et all for graph_dataset), which use the standalone venv fixture added here to test. The mechanisms for making imports optional are quite different though - here we use the pre-existing machinery detect valid outputs, whereas we just more clumsily defer imports for the others, so I though best as separate PRs. |
Ah I see, great. I feel like it would be worth including those commits in this PR. In terms of the aims it's all of a piece, and it's not like this PR is so huge that it needs splitting up for reviewability. That would mean all the work's in one place. And there's a separate question as to whether to leave it as a proof-of-viability for further down the line, or whether to actually merge – though we'd still need to decide how to resolve the pytest issue. |
This means we can run the sandbox w/o pyarrow installed. Introduces a new fixture that installs ehrql in an isolated venv to test the pyarrow-not-installed code paths, which will also be used in subsequent commints testing other optional dependencies.
This defers some sqlalchemy imports and some graphviz imports in ehrql/main.py, in order not to block running the sandbox if they are missing. The primary motiviation was to support running the sandbox in pyodide, but is also just a useful property for ehrql core not to require any dependencies. Have not added tests that error handling works as expected, as we may not move foward with this.
readline is not available in pyodide, it has its own readline implementation which is enable by default in its REPL implementation. Note: cannot test this easily, as linux python comes with readline pre-installed.
bloodearnest
force-pushed
the
minimal-dependencies
branch
from
5bcaba1 to
1adc3cc
Compare
|
Have tidied up the other commits and pushed. We may want a bit more testing polish if we want to land it. |
bloodearnest
changed the title
|
WE've got rid of the sandbox, so the motivation is gone, so closing |
This means we can run the sandbox w/o pyarrow, sqlalchemy, or the various graphviz dependencies installed.
Introduces a new fixture that installs ehrql in an isolated venv to test
the dependency-not-installed code paths work.