open-feature / python-sdk

Python SDK for OpenFeature
https://openfeature.dev
Apache License 2.0
67 stars 18 forks source link

Use requirements.txt instead of requirements-dev #92

Closed hlipsig closed 1 year ago

hlipsig commented 1 year ago

To align with the contents of the readme and python standard practices the requirements.txt file should not be empty. It should match the dev in the main branch. If dev is different it should be in a feature branch to test dependency upgrades.

tcarrio commented 1 year ago

Hey @hlipsig, would you be able to elaborate on this further?

We are currently separating our development and production dependencies within the requirements-dev.in and requirements.in files respectively.

Within the pyproject.toml, we are able to easily define dependencies and project.optional-dependencies.dev packages separately.

I want to make sure we're following best practices, and have seen separate requirements.txt and requirements-dev.txt.

The current setup has some issues with redundancy, as such. It's technically possible for us to forego the requirements.in and requirements-dev.in files if we instead use the pyproject.toml as the source of truth, according to the pip-tools docs.

Perhaps our approach should be to remove these redundancies and use only the pyproject.toml definition alone.

From the pip-tools docs section they cover how this would be done:

# production dependencies
pip-compile -o requirements.txt pyproject.toml

# development dependencies
pip-compile --extra dev -o dev-requirements.txt pyproject.toml
hlipsig commented 1 year ago

Hi. So I'm confused. The .toml file isn't in use during the release process today although it does exist in the project. Having a -dev.txt requirements file and an blank requirements.txt is not pythonic; having two generally isn't pythonic either. I agree with the idea of doing the pip compile to the .toml in the build, but today the .toml isn't used at all. So that's why I didn't recommend the change initially and proposed this instead.

As a python programmer the first thing I did when trying to build your repo was install requirements.txt as per the readme. It was blank. This took me off guard as it's not standard practice. There shouldn't be anything different between dev and production unless it's in a feature branch and even then, it's ideally just 1 file.

Anything required to build from source, or create the binary should be in requirements.txt

I'm curious as to what the driver was on your end to have requirements-dev vs just requirements.txt. Maybe that history will help me make better recommendations.

Maybe we can discuss this on a community meeting. I'll double check when the next one is and make an effort to be there.

tcarrio commented 1 year ago

Hey @hlipsig :wave: Wanted to get back to you on this

Hi. So I'm confused. The .toml file isn't in use during the release process today although it does exist in the project. [...] I agree with the idea of doing the pip compile to the .toml in the build, but today the .toml isn't used at all. So that's why I didn't recommend the change initially and proposed this instead.

Regarding the existing pyproject.toml file. Yes, it's not used as part of the release process. However, it does provide redundant features to our requirements.in and requirements-dev.in files, which define the high-level dependencies for production and development respectively, which we then do version pinning of with pip-tools. Since pip-tools supports pyproject.toml as a definition for input similar to requirements.in, we could do away with those two files and define our requirements in only the pyproject.toml. Personally, that would be the preferred way for us to move forward there.

As a python programmer the first thing I did when trying to build your repo was install requirements.txt as per the readme.

The README.md does not define how to start contributing to the project, for that you can go to the CONTRIBUTING.md file. On our part, we need to fix up our README. This repo is probably the least mature of the OpenFeature org and still needs further refinement, so these types of issues help us identify those :pray:

I'm curious as to what the driver was on your end to have requirements-dev vs just requirements.txt. Maybe that history will help me make better recommendations.

It was important to us to separate the development dependencies only used for CI and local development from the dependencies required to actually use the package. We wanted to use pip-tools to version lock these for reproducible builds, and their docs indicated how this could be done.

Anything required to build from source, or create the binary should be in requirements.txt

I did try to find where this practice is documented officially, but so far it's a lot of fragmented info, which I'll cover next:

Having a -dev.txt requirements file and an blank requirements.txt is not pythonic; having two generally isn't pythonic either.

I had found some conflicting resources for this but I do want us to settle on what is the most Pythonic way to handle dependencies. From projects I have worked on and what documentation and tools are available, it doesn't seem that a single Pythonic way exists or is standardized, but rather many different ways:

Unfortunately, there is not a PEP around this outside of the the requirements file format specification, and many project have different practices in the Python ecosystem. In some cases, it's suggested to have a requirements.txt and requirements-dev.txt, like in the pip-tools docs. Some cases take this even further, with lint requirements also being separated from test requirements. In others, it's expected that the requirements.txt is the full set of dependencies including development tools, and you would use the setup.py with setuptools to configure the "production" dependencies. Other tools such as Pipenv have other practices too, that one making use of a Pipfile and Pipfile.lock which specifies dev and prod dependencies in one file (similar to our thinking for pyproject.toml).

Maybe we can discuss this on a community meeting. I'll double check when the next one is and make an effort to be there.

Definitely agree! We would love to talk about this more. We can schedule a time to go over this in the next session. Let me or @beeme1mr know if you would be able to make it :grin:

federicobond commented 1 year ago

Has this been discussed in a community meeting already? To add my 2c here, I think it's important to consider how different best practices may be for libraries vs end-user applications.

For libraries, dependencies declared in any requirements*.txt files are always dev dependencies, as they are not considered by pip when installing the library for use. A common pattern I have seen for libraries then is to declare any "production" dependencies in the pyproject.toml and then declare dev dependencies in a single requirements.txt file.

A more modern variant of this approach is to replace the requirements.txt file with an optional dependency group in the pyproject.toml as described above, which means doing away with requirements*.txt files altogether, although I am not sure how well supported this approach is for tools like dependabot/renovate.

In both of these cases, pip-tools is no longer needed, as dev dependencies are version pinned and kept updated through renovate/dependabot so contributors know that they are using the same version of their dependencies as everyone else.

federicobond commented 1 year ago

Fixed by https://github.com/open-feature/python-sdk/pull/208