from delphi.epidata.client.delphi_epidata import Epidata
The second use case requires a PyPI install pip install delphi-epidata and then
from delphi_epidata import Epidata
(Note that as it stands, the package isn't installable with the file layout above because delphi-epidata.py isn't in the right directory. When we prepare the package for release on PyPI, our CI first copies the delphi-epidata.py file into the packaging/pypi/delphi_epidata directory, which allows it to be bundled.)
This dual use is handy: the repo code always uses the most recent version of the client code, no installation necessary. And the packaged install is convenient for outside users.
However, with our recent changes here, we fell into a trap: we allowed repo-relative imports to sneak into delphi-epidata.py. Because all our tests rely on repo-relative imports this error snuck by our CI tests, which allowed the standalone client on PyPI to run into import errors.
Proposal
We should always install and import the client as a package. Repo code would need to declare ./src/client/packaging/pypi/ as a local dependency in our requirements files and replace the repo-relative imports with package imports. To make this work, there are two options:
either we just move delphi-epidata.py into the packaging/pypi/delphi_epidata/ directory
or we move it into that directory at Docker build time
The benefits of the first approach are simplicity, but it suffers from the drawback that other code (not in this repo) uses repo-relative imports; moving this file would break those imports. The benefit of the second approach is keeping that code functional, while the drawback is increased complexity. If the code in those repos relies on the same Docker images, then we'd need to think some more.
With this change in place we would:
make sure that the tests in this repo test the package as it appears for our PyPI users (and avoid bugs like the above)
simplify our CI installation procedure by removing the file copying step
A snag to consider:
if you're doing local development on the client, the Dockerfile would need to get rebuilt every time you make a change to the file; it would be nice to find some way to install the package in an editable way that doesn't trigger a rebuild
Background
The Python client
delphi-epidata.pyhas a peculiar file layout:I think it's structured this way to allow importing the client in this repo as a module and by outside users via PyPI as an installable package.
The first use case can be seen here:
The second use case requires a PyPI install
pip install delphi-epidataand then(Note that as it stands, the package isn't installable with the file layout above because
delphi-epidata.pyisn't in the right directory. When we prepare the package for release on PyPI, our CI first copies thedelphi-epidata.pyfile into thepackaging/pypi/delphi_epidatadirectory, which allows it to be bundled.)This dual use is handy: the repo code always uses the most recent version of the client code, no installation necessary. And the packaged install is convenient for outside users.
However, with our recent changes here, we fell into a trap: we allowed repo-relative imports to sneak into
delphi-epidata.py. Because all our tests rely on repo-relative imports this error snuck by our CI tests, which allowed the standalone client on PyPI to run into import errors.Proposal
We should always install and import the client as a package. Repo code would need to declare
./src/client/packaging/pypi/as a local dependency in our requirements files and replace the repo-relative imports with package imports. To make this work, there are two options:delphi-epidata.pyinto thepackaging/pypi/delphi_epidata/directoryThe benefits of the first approach are simplicity, but it suffers from the drawback that other code (not in this repo) uses repo-relative imports; moving this file would break those imports. The benefit of the second approach is keeping that code functional, while the drawback is increased complexity. If the code in those repos relies on the same Docker images, then we'd need to think some more.
With this change in place we would:
A snag to consider: