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

Configure the Project to be Buildable #252

Merged
merged 4 commits into from
Nov 22, 2021
Merged

Configure the Project to be Buildable #252

merged 4 commits into from
Nov 22, 2021

Conversation

ghickman
Copy link
Contributor

@ghickman ghickman commented Nov 19, 2021

This sets up the requisite config to make v2 pip installable. There are a couple of commits to change how the CLI and an import work to make that happen.

I've opted to use modern build tooling because we're not tied to older python versions or environments (yet). The metadata was generated by flit locally, but we don't need it installed to build artefacts, build can handle that, and pip install works without either. All build metadata is contained in pyproject.toml, and documented by flit here. Production requirements have been moved to pyproject.toml, and the Justfile updated to use pyproject.toml as the spec file for pip-compiling prod requirements.

@benbc
Copy link
Contributor

benbc commented Nov 19, 2021

@ghickman I don't have time to review this now, but we should check that IDEs understand how to install dependencies from pyproject.toml. I can check PyCharm on Monday.

@ghickman
Copy link
Contributor Author

I've been testing this locally with a clean virtualenv and installing from the tarball of this branch:

rm -rf venv
python -m venv venv
venv/bin/python -m pip install https://github.com/opensafely-core/cohort-extractor-v2/archive/refs/heads/make-buildable.tar.gz#egg=cohortextractor
venv/bin/cohortextractor -h

@ghickman
Copy link
Contributor Author

Discussion with @bloodearnest led to:

  • discovering pip-compile --output-file=requirements.prod.txt pyproject.toml is a thing we can do
  • reworking the PR to use that, which reverts all the Dockerfile and nearly all Justfile changes

cohortextractor/__main__.py Outdated Show resolved Hide resolved
requirements.prod.txt Outdated Show resolved Hide resolved
cohortextractor/__main__.py Outdated Show resolved Hide resolved
@ghickman ghickman force-pushed the make-buildable branch 2 times, most recently from 027eabd to 7de8357 Compare November 22, 2021 09:25
@ghickman
Copy link
Contributor Author

we should check that IDEs understand how to install dependencies from pyproject.toml.

@benbc – I didn't think too much about this last week, but is that a common flow? I've not come across it in Python land! I would expect it to work because the IDE must still be using a local venv/pip to install things? (surely 😰) We should still check of course 🙂

@ghickman ghickman force-pushed the make-buildable branch 2 times, most recently from c6ba118 to d18779a Compare November 22, 2021 10:53
To use entrypoints for a built artefact we need to be able to call the
main() function directly, so we move the reading of cli args into it.
However we don't want to clutter tests with mocks so this sets an empty
default and only populates it if there aren't any args passed in.
@benbc
Copy link
Contributor

benbc commented Nov 22, 2021

@ghickman Having looked at this a bit more, I think PyCharm is just integrating with the venv. I think it can do some pip-compile stuff too, but that's much less important. Nothing for you to worry about here.

@ghickman ghickman merged commit 605372e into main Nov 22, 2021
@ghickman ghickman deleted the make-buildable branch November 22, 2021 12:21
evansd added a commit that referenced this pull request May 9, 2024
They were moved in as part of #252 on the basis that this was "more
modern". However we don't – and don't intend to – publish ehrQL as a
package to PyPI and when we do build it as a package in our Docker image
we tell pip to ignore dependencies anyway. And for some reason that we
haven't got to the bottom of, Dependabot is unable to update our
production dependencies if we specify them in this way.

This moves us back to the `requirements.prod.in` pattern that we use
elsewhere and which Dependabot handles nicely.

Closes #964
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.

3 participants