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

Move requirements*.txt files into pyproject.toml #552

Open
mfisher87 opened this issue Aug 12, 2024 · 13 comments · May be fixed by #565
Open

Move requirements*.txt files into pyproject.toml #552

mfisher87 opened this issue Aug 12, 2024 · 13 comments · May be fixed by #565
Labels
enhancement New feature or request

Comments

@mfisher87
Copy link
Member

mfisher87 commented Aug 12, 2024

I think it's more conventional to express these as extras groups in the project metadata. Then they can be installed as desired and in combination, e.g. with pip install --editable .[dev,docs]

https://github.com/scientific-python/cookie/blob/0dd0032a08231ed246caccfd6be4b7e82466fe51/pyproject.toml#L38-L51

@weiji14
Copy link
Member

weiji14 commented Aug 13, 2024

Once upon a time, there was a good reason to keep the requirements.txt file in the top level folder because that was what GitHub used to create the 'Dependency graph' according to https://docs.github.com/en/code-security/supply-chain-security/understanding-your-software-supply-chain/about-the-dependency-graph#supported-package-ecosystems.

image

Nowadays, I think that's not true (at least I can see dependents on my repo that doesn't have a requirements.txt file), so we could probably remove 'requirements.txt' if needed. But happy with just removing requirements-dev.txt and requirements-docs.txt only (will need to update the docs and CI scripts relying on it).

@mfisher87
Copy link
Member Author

I'm surprised the doc wasn't updated!

@mfisher87 mfisher87 added the enhancement New feature or request label Aug 13, 2024
@mfisher87 mfisher87 changed the title Move requirements*.txt files into pyproject.toml? Move requirements*.txt files into pyproject.toml Aug 13, 2024
@JessicaS11
Copy link
Member

Then they can be installed as desired and in combination, e.g. with pip install --editable .[dev,docs]

Can you also install this way into a conda environment? Maybe I'll understand better after Don and Romina's packaging tutorial, but this has historically been a topic that feels extra challenging to me (which I interpret to mean it's likely even more challenging for other less technically-interested researchers). I can easily create a conda environment with multiple requirements files. My recollection is I cannot do that if the requirements are all only in the metadata.

we could probably remove 'requirements.txt' if needed.

We'd need to then add an environment file for users (and update the docs accordingly).

@mfisher87
Copy link
Member Author

mfisher87 commented Aug 13, 2024

Oh my goodness, yes, this is challenging for me as well. I want to be grateful for Conda Forge for a moment, it's been so good for how I think about packaging! 🪨 ⭐

Can you also install this way into a conda environment?

Yes, if the conda environment has pip installed! How are you currently installing requirements.txt into a conda environment?

My personal opinion: this project is a pure python library which does not depend on conda packages, but we do expect conda users to be both contributors and users of this package, as well as possibly non-conda users. For the purposes of this discusison, I think we can ignore users -- sorry, still love you ❤️ but we serve them separately by packaging for conda-forge.

I think we can best serve this audience of contributors by doing things in a way that's most portable to and from other projects, which may look like two steps: (1) Create a new environment with your environment manager. We'll show an example of how to do this with conda. (2) Install the package using pip into the new environment.

# 1
conda create icepyx-dev pip
conda activate icepyx-dev
# 2
pip install --editable .[dev,docs]

What do you think?

@JessicaS11
Copy link
Member

Yes, if the conda environment has pip installed! How are you currently installing requirements.txt into a conda environment?

This is a deal breaker for me. I want to be able to create my environment with all potential dependencies and only have to pip install icepyx -e (emphasis on the editable part). I've had enough headaches to last my lifetime with pip installing too many packages after creating a skeleton new conda environment. I'm not 100% closed off on this idea, but I need to do a lot more reading and learning before I'm comfortable going ahead with it.

@JessicaS11
Copy link
Member

JessicaS11 commented Aug 16, 2024

I'm not sure what changes you're requesting here or what the dealbreaker is

I'm saying I'm not comfortable with the PR title: "Move requirements*.txt files into pyproject.toml"

@mfisher87
Copy link
Member Author

I'm still not sure I understand, I'm sorry. The PR is titled "Move dependencies from requirements files into pyproject.toml, add extra groups for dev, docs". What would you prefer for the title?

@JessicaS11
Copy link
Member

I'm saying I'm not comfortable with the PR title: "Move requirements*.txt files into pyproject.toml"

I see that this isn't necessarily any clearer - sorry about that. It's not the actual title, but the task described by the title. Literally, I'm not comfortable with "mov[ing] requirements*.txt files into pyproject.toml".

@mfisher87
Copy link
Member Author

mfisher87 commented Aug 20, 2024

Thanks for clarifying, that makes sense to me. It sounds like you need more info, but aren't ruling this out, correct?

Under that assumption, I'd suggest checking out:

@weiji14
Copy link
Member

weiji14 commented Aug 21, 2024

Not to take sides, but I think I get @JessicaS11's concern that the shift here means that we will be mixing conda-forge dependencies with pip dependencies. The current development environment setup uses this command:

mamba env create --name icepyx-env --channel conda-forge -f requirements-dev.txt -f requirements.txt

What this command does, is actually install the dependencies listed in requirements*.txt from conda-forge. We could just install Python via conda-forge and the other dependencies via PyPI, but keeping this mixed conda/pip environment intact requires a level of discipline, and if you accidentally run conda install xxx in that environment, things will break. The best case is still to get everything from conda-forge and let the conda solver handle dependency conflicts rather than trusting that conda/pip's resolvers play nicely with each other.

@mfisher87
Copy link
Member Author

mfisher87 commented Aug 22, 2024

First: All of the below are my opinions, and if I'm not convincing anyone, then no problem!

What this command does, is actually install the dependencies listed in requirements*.txt from conda-forge.

True (well, partly... the command doesn't actually work, but I see what the intent is)! This is part of my concern -- as this is a pure Python package, the conda-forge distribution is downstream of our build target PyPI. I feel it's unusual and unintuitive that the development stage would occur with dependencies from a distribution downstream of our build target.

We could just install Python via conda-forge and the other dependencies via PyPI, but keeping this mixed conda/pip environment intact requires a level of discipline, and if you accidentally run conda install xxx in that environment, things will break.

Agreed, there are downsides to this approach, but personally I feel less than the current approach. I think it's OK to use conda to create environments and use pip to install into them if you know to use conda as an environment manager, not a dependency manager.

IMO the best approach overall is to not primarily document for contributors using conda to avoid this confusion. If we want conda contributor documentation, I think we should treat it as an exception ("if you want to use conda..."), and clarify that distinction between dependency management and environment management. Our documentation already mixes conda and pip commands without warning of the possible problems, so in my view this would be an improvement! (There is no way to editable install with conda itself, they dropped support for conda develop like half a decade ago (I dunno lol), it was calling out to pip anyway).

The best case is still to get everything from conda-forge and let the conda solver handle dependency conflicts rather than trusting that conda/pip's resolvers play nicely with each other.

Under the assumption that all contributors will use conda, I agree, but I think it's important to make contributing accessible to people who use a more conventional pure Python development toolchain instead of conda. Of course, without disadvantaging contributors who want to use conda.

I'd be happy to update the docs on this PR if it helps to "show, don't tell" what I'm proposing.

@JessicaS11
Copy link
Member

I've done some reading and, though I cannot find any good arguments for WHY one should do dependency resolution with pip over conda (even for a pure python package), I can see the logic for having optional dependencies all contained within pyproject.toml instead of separate files. So while I am not enthusiastically in favor of this transition, I am okay with moving forward on it.

Under the assumption that all contributors will use conda, I agree, but I think it's important to make contributing accessible to people who use a more conventional pure Python development toolchain instead of conda. Of course, without disadvantaging contributors who want to use conda.

My personal perspective here is that a pure Python developer is more likely to be comfortable navigating across pip + conda (and can always pip install -f requirements.txt -f requirements-dev.txt), while a contributor using conda is less likely to have an intuition for using (e.g.) pip install icepyx[dev] or understanding why their mixed pip + conda environment isn't working. Since there's never going to be a solution that makes everyone happy, the best we can do is make sure our docs are very clear that those contributing may need to install their dev environment differently than they might otherwise (if they're just using conda). A short list of what we'll need to mention includes (this is as much a mental note for myself as anything else):

  • basic installation for all users: recommending a pip install (with a note that a conda install is also an option and reminder that users should stick to whichever one they first used for additional installs in that environment)
  • for contributors: using pip install -e .[dev] (after creating a base environment with conda; as noted in the start of the thread), or better yet with all steps completed via a top-level environment.yml file (e.g.).
  • what pre-configured installation options are available to the user (e.g. dev, visualization, docs, complete)

@mfisher87
Copy link
Member Author

mfisher87 commented Sep 5, 2024

So while I am not enthusiastically in favor of this transition, I am okay with moving forward on it.

I don't want to push through on this if you don't feel it's in the project's best interest. I may have strong opinions, but I'm OK with holding onto them weakly :)

I cannot find any good arguments for WHY one should do dependency resolution with pip over conda (even for a pure python package)

I don't know if it's a "good" argument or not :) but my thinking on this is almost entirely about reducing complexity. When installing a dev environment from a downstream distribution (conda-forge) of our actual target (PyPI), it's possible for unexpected differences in those distributions to make supporting build errors or users of the PyPI distribution more complex. It's just easier for me personally to reason about

flowchart LR

  PyPI --> dev
  dev --> PyPI
  PyPI --> conda-forge
Loading

than

flowchart LR

  conda-forge --> dev
  dev --> PyPI
  PyPI --> conda-forge
Loading

In the former model, we only think about conda-forge in the context of the feedstock, but in the latter, we need to think about it in our development loop as well.

better yet with all steps completed via a top-level environment.yml file

💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants