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

Consider standardizing Python client packaging #1468

Open
dshemetov opened this issue Jun 4, 2024 · 0 comments
Open

Consider standardizing Python client packaging #1468

dshemetov opened this issue Jun 4, 2024 · 0 comments
Labels
code health readability, maintainability, best practices, etc python client changes the Python client refactor Substantial projects to make the code do the same thing, better.

Comments

@dshemetov
Copy link
Contributor

dshemetov commented Jun 4, 2024

Background

The Python client delphi-epidata.py has a peculiar file layout:

packaging/
  pypi/
    delphi_epidata/
      __init__.py
    .gitignore
    CHANGELOG.md
    LICENSE
    README.md
    setup.py
delphi-epidata.py

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:

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
@melange396 melange396 added refactor Substantial projects to make the code do the same thing, better. python client changes the Python client code health readability, maintainability, best practices, etc labels Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health readability, maintainability, best practices, etc python client changes the Python client refactor Substantial projects to make the code do the same thing, better.
Projects
None yet
Development

No branches or pull requests

2 participants