conda-incubator / conda-store

Data science environments, for collaboration. ✨
https://conda.store
BSD 3-Clause "New" or "Revised" License
137 stars 44 forks source link

DEV - Clean and update dependencies #792

Closed trallard closed 3 months ago

trallard commented 3 months ago

Thanks to @pavithraes we realised that now runtime dependencies for conda-store(-server) are not up to date nor included in distributed versions of the package.

This is in part due to having bloated environment.yaml files that contain both dev and runtime dependencies (that should only de specified in the pyproject.toml). Since these are both used in local development and CI missing dependencies have gone unnoticed. This PR aims to clean the dependencies and enviroments.

Description

This pull request:

Pull request checklist

Additional information

How to test

netlify[bot] commented 3 months ago

Deploy Preview for kaleidoscopic-dango-0cf31d canceled.

Name Link
Latest commit 48ec413f8e98df06715ceff3f4f3f8a2d3d60c1f
Latest deploy log https://app.netlify.com/sites/kaleidoscopic-dango-0cf31d/deploys/65fb1ad491da1900084dd14d
netlify[bot] commented 3 months ago

Deploy Preview for kaleidoscopic-dango-0cf31d canceled.

Name Link
Latest commit 16911adaf09a82f1685b2d5461ab2113983351ba
Latest deploy log https://app.netlify.com/sites/kaleidoscopic-dango-0cf31d/deploys/6601b3bf836dc800085cc60d
trallard commented 3 months ago

This PR makes too many unrelated changes: whitespace, documentation, version bumps, formatting, which makes it harder to review

Documentation is not unrelated -> addresses changes to the workflow and adds missing installation instructions.

Whitespaces / formatting -> this sneaked in through pre-commit.ci before I could make the updates to the config 🀷🏽 I can revert this as it is the only "unrelated" change

I suggest keeping only absolutely essential things here (for the purposes of adding missing dependencies to the pyproject file): env yaml files, workflows, pyproject update, and splitting out everything else into one or more self-contained PRs

Everything is mushed together atm; I can split it into smaller PRs, which is not a problem, but all are ultimately dependencies.

Moreover, I think the current approach needs rethinking altogether. The initial goal of this PR is to add missing dependencies to the pyproject file. But as is, it tries to change the existing testing infra, which relies heavily on env yaml files in several places, which is also what devs use to test and run the code locally (as opposed to using hatch to build via pyproject)

It started by "trying to add" missing dependencies, but without adequate approaches to how these are added, tested, and used, it does not address the root cause - which is all dependencies are treated the same, and we rely solely on yaml environment files for all the needs. This does not change anything from using yaml env files to hatch at all. Regardless of whether you use environment-dev.yaml to set your local environment, you still need to do pip installβ€”e . This PR keeps the use of the yaml environment files but separates dev dependencies from conda-store and conda-store-server, the underlying workflows and processes remain unchanged. For contributors/maintainers at most this PR would mean they need to update their local development environment, but there are no material changes to anything else.

Changing too many things at once is error-prone. So I suggest this instead: add missing dependencies to the pyproject file as needed, add a completely separate workflow that would use the pyproject file and would run unit tests against conda-store installed via hatch, keep everything else as is. I understand this is not ideal because of duplication. But this way we keep the good known baseline around until we have a working solution with hatch. After a while, once we're comfortable with new testing infra, we can retire the old one.

There is no new and old testing infra; it is just cleaning where dependencies are declared and sourced.

trallard commented 3 months ago

Will close this as all the additions here have been split across other PRs