Skip to content

Conversation

ds-cbo
Copy link

@ds-cbo ds-cbo commented Sep 17, 2024

Pull Request check-list

  • Do tests and lints pass with this change?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Is there an example added to the examples folder (if applicable)?

Description of change

I upgraded the deprecated setup.py to the modern pyproject.toml format, enabling modern installer tools.

@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 75.16%. Comparing base (c8c19c2) to head (fcb7b67).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
setup.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #96      +/-   ##
==========================================
+ Coverage   75.12%   75.16%   +0.04%     
==========================================
  Files         132      132              
  Lines       34398    34480      +82     
==========================================
+ Hits        25840    25918      +78     
- Misses       8558     8562       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mkmkme
Copy link
Collaborator

mkmkme commented Sep 18, 2024

Thanks for your contribution.

One question though: why do you think setup.py is deprecated? This web page explicitly says it is not.

@ds-cbo
Copy link
Author

ds-cbo commented Sep 18, 2024

Interesting, those articles contradict what I see other big projects doing, even if they keep using setuptools as build system:

Internally we also moved to pyproject by removing setup.py because we believed that was the new way to go, I didn't even know that you could keep both files in tandem.

Would you like me to bring back a barebones from setuptools import setup ; setup() setup.py?

@aiven-sal
Copy link
Member

Did you encounter any specific issues with setup.py or is this intended more as a cosmetic change?

@aiven-sal
Copy link
Member

BTW yes, please don't remove setup.py completely.

As a side note, the sign-off thingy should contain a real name and a real email address

@ds-cbo
Copy link
Author

ds-cbo commented Sep 18, 2024

@aiven-sal I encountered issues with our build system which requires pyproject.toml be available in libraries we use. Of course we could also put some patches in our repo only, but I imagined it might be useful to more people to contribute upstream too.

I've added the setup.py in the last commit.

What do you mean with "real name" and "real email address"? These are the two linked to this GitHub account, I imagined those would be more useful than some generic untraceable values.

@aiven-sal
Copy link
Member

What do you mean with "real name" and "real email address"? These are the two linked to this GitHub account, I imagined those would be more useful than some generic untraceable values.

Maybe this can clarify what I mean: https://github.com/cncf/foundation/blob/main/dco-guidelines.md#dco-and-real-names
The "real name" doesn't necessarily have to be the name on your ID, but it shouldn't be an "anonymous id" and the email needs to be an actual email address that can be used to contact you directly.

@aiven-sal
Copy link
Member

To put it in a different way. Imagine we just met at a conference, what name would you use to introduce yourself? That should be the name you use in signoff

Signed-off-by: ds-cbo <82801887+ds-cbo@users.noreply.github.com>
Signed-off-by: C. Bo <cbo@dreamsolution.nl>
Signed-off-by: ds-cbo <82801887+ds-cbo@users.noreply.github.com>
Signed-off-by: C. Bo <cbo@dreamsolution.nl>
Signed-off-by: ds-cbo <82801887+ds-cbo@users.noreply.github.com>
Signed-off-by: C. Bo <cbo@dreamsolution.nl>
Signed-off-by: ds-cbo <82801887+ds-cbo@users.noreply.github.com>
Signed-off-by: C. Bo <cbo@dreamsolution.nl>
@ds-cbo
Copy link
Author

ds-cbo commented Sep 18, 2024

I guess that would be it then?

@mkmkme
Copy link
Collaborator

mkmkme commented Sep 20, 2024

The CI has failed. Could you please fix it accordingly?

It seems that:

  1. Words pyproject and toml should be added to the dictionary
  2. The CI is using python setup.py directly which is no longer correct

Signed-off-by: ds-cbo <82801887+ds-cbo@users.noreply.github.com>
@ds-cbo
Copy link
Author

ds-cbo commented Sep 20, 2024

@mkmkme I've just updated the dictionary, but as far as I know I already replaced all instances of python setup.py from CI in a previous commit. Am I still missing something (and if so: where?), or were you perhaps looking at an older CI run?

Do you also expect me to pass the (seemingly unrelated and/or outdated?) pip-audit checks?

@mkmkme
Copy link
Collaborator

mkmkme commented Sep 23, 2024

@ds-cbo There's still a bunch of leftovers that you can find by https://github.com/search?q=repo%3Avalkey-io%2Fvalkey-py%20setup.py&type=code

IMO all of them should be fixed.
The one that is causing the CI to fail is under .github/workflows. The ones in INSTALL and tasks.py are user-facing, so I'd change them, too.

Thanks! :)

@mkmkme
Copy link
Collaborator

mkmkme commented Sep 23, 2024

Do you also expect me to pass the (seemingly unrelated and/or outdated?) pip-audit checks?

We certainly can't merge the PR with CI failing. If you don't want to handle this, I can do it myself.

@mkmkme
Copy link
Collaborator

mkmkme commented Sep 23, 2024

I am also quite concerned that the direct calls to python setup.py from the CI are failing despite the fact we do have setup.py now

Signed-off-by: ds-cbo <82801887+ds-cbo@users.noreply.github.com>
@ds-cbo
Copy link
Author

ds-cbo commented Sep 23, 2024

@ds-cbo There's still a bunch of leftovers that you can find by https://github.com/search?q=repo%3Avalkey-io%2Fvalkey-py%20setup.py&type=code

IMO all of them should be fixed. The one that is causing the CI to fail is under .github/workflows. The ones in INSTALL and tasks.py are user-facing, so I'd change them, too.

Thanks! :)

As far as I know, I've already modified those in earlier commits. See for example https://github.com/ds-cbo/valkey-py/blob/pyproject.toml/.github/workflows/install_and_test.sh#L23

We certainly can't merge the PR with CI failing. If you don't want to handle this, I can do it myself.

Understandable. I wouldn't mind fixing it, but I don't really know your preferred course of action here. It's failing on Babel 2.8.0 even though 2.16.0 is already available and nothing seems to prevent it from being installed. Maybe sphinx<7.0 in our dependency list is blocking it? The last sphinx<7.0 would be 6.2.1 from april 2023. I have no clue what the impact of that major upgrade would be on this project though, so I don't want to mindlessly remove the upper pin.

I'm guessing this change happened because now we're running pip-audit including the docs dependencies, where previously we'd only run it on dev? I'm a bit unsure

I am also quite concerned that the direct calls to python setup.py from the CI are failing despite the fact we do have setup.py now

I still think you're looking at older CI reports, the reason the newest packaging runs were failing was because build wasn't available, and I've just pushed a commit to fix that.

@mkmkme mkmkme self-assigned this Sep 24, 2024
Comment on lines +82 to +97
[tool.setuptools.packages.find]
include = [
"valkey",
"valkey._parsers",
"valkey.asyncio",
"valkey.commands",
"valkey.commands.bf",
"valkey.commands.json",
"valkey.commands.search",
"valkey.commands.timeseries",
"valkey.commands.graph",
"valkey.parsers",
]

[tool.setuptools.package-data]
valkey = ["py.typed"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we do not need this anymore after we migrate from setup.py to pyproject.toml

@@ -1,62 +1,3 @@
#!/usr/bin/env python
from setuptools import find_packages, setup
from setuptools import setup
Copy link
Contributor

@trim21 trim21 May 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we also do not need this file if it just call setup() without any arguments

Comment on lines +96 to +97
[tool.setuptools.package-data]
valkey = ["py.typed"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[tool.setuptools.package-data]
valkey = ["py.typed"]

@trim21
Copy link
Contributor

trim21 commented May 10, 2025

I would suggest we use flit-core and stop using setuptools since it's a pure python package, then we can get ride of the long tool.setuptools.packages.find list

@amirreza8002
Copy link
Contributor

i moved django-valkey to a uv + hatchling system
and it's definitely an improvement

imo pyproject should be used for everything, having all configs in one place, and with a readable and maintainable format is a good thing
it also makes it easier for people who want to contribute for the first time

i do suggest uv + hatch, and if you want i can come with a patch

but regardless of that, pyproject is a good move

@amirreza8002
Copy link
Contributor

tho admittedly valkey-py has a sane packaging setup, and it's not a necessity to move, unlike some packages like celery and aiocache

but it'll still be a good addition and i think everyone will be happier with it, but i do as you wish

@trim21
Copy link
Contributor

trim21 commented May 11, 2025

hatchling still has it's own dependencies so I would suggest flit-core.

but any one is better than setuptools

@amirreza8002
Copy link
Contributor

amirreza8002 commented May 11, 2025

since we aren't adding hatch as a dependency there is no need to install anything
uv uses hatch when packaging, and packaing only happens every once in a while, by github's CI

i don't know much about build backends, setuptools broke my package so i moved to hatch, and it works
i don't insist on it

@trim21
Copy link
Contributor

trim21 commented May 11, 2025

since we aren't adding hatch as a dependency there is no need to install anything
uv uses hatch when packaging, and packaing only happens every once in a while, by github's CI

not really, there are 2 things here, the hatchling and the hatch. I guess no one suggest use hatch so I'll skip it.

The build-system hatchling defined in build-system.requires, which will be pulled with it's full dependencies tree from pypi or cache every time the ci run pip install . (or uv pip) on source.

You can take it as , the pip/uv will create a new venv, install all package defined in build-system.requires and call some api to from build-system.build-backend to build a wheel from source, then install that wheel in to current venv, of read metadata from it when you run uv sync.

@amirreza8002
Copy link
Contributor

i don't think we ever have to use uv pip or pip install . in the CI,
the only time hatchling is needed is when packaging, unless i've misunderstood something

and even if hatchling is used in other places, it doesn't seem to have a heavy dependency list, also it's cached in github's servers
and it's not something users would have to worry about (nor the package maintainers/contributes)

anyway, as i said, i don't insist, i suggested it because it's what i've used

@trim21
Copy link
Contributor

trim21 commented May 11, 2025

i don't think we ever have to use uv pip or pip install . in the CI,
the only time hatchling is needed is when packaging, unless i've misunderstood something

I think we will have pip install '.[dev,docs]' in CI after we migrate to setuptools?
even if we use uv sync(unless with frozen) or uv lock, it's the same process that the package manager will build a wheel from source first to extract meta data like dependencies.

and it's not something users would have to worry about (nor the package maintainers/contributes)

yeah, in nowadays the users of valkey-py should always use wheel, especially valkey-py is a pure python package, they don't need care about build-system.

I'm worrying about that some dependency may have breaking change in the future (so the old sdist would be broken then), but maybe I'm worrying for somethine that not worth worried about.

@amirreza8002
Copy link
Contributor

yup you seem to be correct

i don't think dependencies are a big deal
but the whole subject is not a big deal in general

i'll wait to see if maintainers are even intrested :), that's what matters

@mkmkme
Copy link
Collaborator

mkmkme commented May 20, 2025

Hey,

Overall the proper way to update setup.py to pyproject.toml has been in my todo for a while. I would really-really like to do that whilst ensuring that we won't break anything for anyone even with the oldest Python supported or with the most exotic way this package could be installed. So my original thought was to not move away from setuptools but make pyproject.toml use it while also keeping dummy setup.py that could be used (in case someone still installs it via python3 setup.py install or something).

Thanks for your suggestion, though! I'll definitely look it up and will make a mental pinpoint on this

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.

6 participants