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

Introduced pyproject.toml and moved static metadata from setup.py #1592

Merged
merged 5 commits into from
Oct 18, 2024

Conversation

pranavrth
Copy link
Member

@pranavrth pranavrth commented Jun 20, 2023

  • Introduced pyproject.toml
  • Moved static metadata from setup.py
  • Removed Python 3.6 support
  • Updated RELEASE.md

@pranavrth pranavrth marked this pull request as ready for review June 23, 2023 06:10
@pranavrth pranavrth requested a review from a team as a code owner June 23, 2023 06:10
@cla-assistant
Copy link

cla-assistant bot commented Aug 15, 2023

CLA assistant check
All committers have signed the CLA.

@pranavrth pranavrth force-pushed the dev_pyproject-toml branch 2 times, most recently from fea7154 to 665b2f0 Compare October 27, 2023 11:09
@pranavrth pranavrth requested a review from emasab October 27, 2023 11:55
@pranavrth pranavrth force-pushed the dev_pyproject-toml branch from 91878fe to 3265136 Compare April 18, 2024 11:19
Copy link
Contributor

@emasab emasab left a comment

Choose a reason for hiding this comment

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

Given we're touching project dependencies we should do a cleanup of them, here's what I suggest

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated
[project.urls]
Homepage = "https://github.com/confluentinc/confluent-kafka-python"

[project.optional-dependencies]
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid duplicating dependencies we can move all requirements.txt to the project root folder and use them as dynamic dependencies, like:

[project]
dynamic = ["dependencies", "optional-dependencies"]

[tool.setuptools.dynamic]
dependencies = {file = ["requirements.txt"]}
optional-dependencies.schemaregistry = { file = ["requirements-schemaregistry.txt"] }
optional-dependencies.avro = { file = ["requirements-avro.txt", "requirements-schemaregistry.txt"] }

Copy link
Contributor

Choose a reason for hiding this comment

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

You cannot reference
-r requirements-schemaregistry.txt in requirements-avro.txt so you can a requirements-dev-install.txt that lists the dependencies for the CI

-r requirements-schemaregistry.txt
-r requirements-avro.txt
-r requirements-json.txt
-r requirements-protobuf.txt
-r requirements-dev.txt

Copy link
Contributor

Choose a reason for hiding this comment

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

We should also change tox.ini to use
pip install .[dev]

deps =
    -rrequirements-dev-install.txt

and add py311. py312 lacks of the package pkg_resources needed by Trivup

Copy link
Contributor

Choose a reason for hiding this comment

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

Also replace this in DEVELOPER.md#Generate Documentation

Install doc dependencies:

    $ pip install .[doc]

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use requirements-dev-install.txt when scripts are requiring different requirements files

Copy link
Contributor

Choose a reason for hiding this comment

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

Once we install with .[doc] we can remove this import in docs/conf.py
sys.path[:0] = [os.path.abspath(x) for x in glob('../build/lib.*')]

setup.py Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
MANIFEST.in Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@pranavrth pranavrth force-pushed the dev_pyproject-toml branch from 3265136 to 2118262 Compare May 9, 2024 10:38
@pranavrth pranavrth requested a review from a team as a code owner July 1, 2024 10:08
@pranavrth pranavrth force-pushed the dev_pyproject-toml branch from d955327 to 9bbe5f6 Compare July 1, 2024 10:08
@confluent-cla-assistant
Copy link

🎉 All Contributor License Agreements have been signed. Ready to merge.
Please push an empty commit if you would like to re-run the checks to verify CLA status for all contributors.

@@ -1,3 +1,3 @@
fastavro>=0.23.0
Copy link
Contributor

Choose a reason for hiding this comment

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

There was a problem with fastavro and 3.7 in versions higher than 1.8.0, but maybe it's been fixed in 1.8.1

Suggested change
fastavro>=0.23.0
fastavro < 1.8.0; python_version == "3.7"
fastavro < 2; python_version > "3.7"

@@ -1,14 +1,17 @@
# core test requirements
urllib3<2.0.0;python_version<="3.7"
urllib3
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
urllib3
urllib3 >= 2.0.0,<3; python_version > "3.7"

Comment on lines 10 to 17
# other requirements
fastavro<1.8.0;python_version=="3.7"
fastavro>=1.8.4;python_version>"3.7"
fastavro
requests
avro>=1.11.1,<2
jsonschema
protobuf
protobuf>=3.6.0
Copy link
Contributor

Choose a reason for hiding this comment

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

These dependencies would be duplicate, please remove them and add a requirements/requirements-tests-install.txt that one can use to install test dependencies plus SR ones.

-r requirements-tests.txt
-r requirements-schemaregistry.txt
-r requirements-avro.txt
-r requirements-protobuf.txt
-r requirements-json.txt

tests/README.md Outdated
Comment on lines 21 to 22
$ pip install -r requirements/requirements-tests.txt
$ python3 -m pip install .
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
$ pip install -r requirements/requirements-tests.txt
$ python3 -m pip install .
$ python3 -m pip install .[tests]

@@ -8,7 +8,7 @@ this_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"


# Skip PyPy, Python2, old Python3 versions, musl, and x86 builds.
export CIBW_SKIP="pp* cp27-* cp35-* *i686 *musllinux* $CIBW_SKIP"
export CIBW_SKIP="pp* cp27-* cp35-* cp36-* *i686 *musllinux* $CIBW_SKIP"
Copy link
Contributor

Choose a reason for hiding this comment

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

Below, replace
so that setup.py finds librdkafka.
with
so that cibuildwheel finds librdkafka.

Copy link
Contributor

Choose a reason for hiding this comment

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

In RELEASE.md there are these lines to change (confluent_kafka.h and pyproject.toml)

Commit these changes with a commit-message containing the version:

    $ git commit -m "Version v0.11.4rc1" src/confluent_kafka/src/confluent_kafka.h docs/conf.py pyproject.toml

Copy link
Contributor

Choose a reason for hiding this comment

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

Remember to change the confluent_kafka.c to confluent_kafka.h as it's what was modified just before and needs to be committed.

pyproject.toml Outdated
readme = "README.md"
license = { file = "LICENSE" }
requires-python = ">=3.7"
dynamic = ["optional-dependencies","dependencies"]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
dynamic = ["optional-dependencies","dependencies"]
dynamic = ["dependencies", "optional-dependencies"]

pyproject.toml Outdated
optional-dependencies.avro = { file = ["requirements/requirements-avro.txt", "requirements/requirements-schemaregistry.txt"] }
optional-dependencies.json = { file = ["requirements/requirements-json.txt", "requirements/requirements-schemaregistry.txt"] }
optional-dependencies.protobuf = { file = ["requirements/requirements-protobuf.txt", "requirements/requirements-schemaregistry.txt"] }
optional-dependencies.dev = { file = ["requirements/requirements-tests.txt", "requirements/requirements-docs.txt", "requirements/requirements-examples.txt"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the SR dependencies to dev (for testing) and break these long lines.
As it's not possible to use requirements-tests-install.txt, containing -r, here:

Suggested change
optional-dependencies.dev = { file = ["requirements/requirements-tests.txt", "requirements/requirements-docs.txt", "requirements/requirements-examples.txt"] }
optional-dependencies.dev = { file = [
"requirements/requirements-docs.txt",
"requirements/requirements-examples.txt",
"requirements/requirements-tests.txt",
"requirements/requirements-schemaregistry.txt",
"requirements/requirements-avro.txt",
"requirements/requirements-json.txt",
"requirements/requirements-protobuf.txt"] }

pyproject.toml Outdated
optional-dependencies.json = { file = ["requirements/requirements-json.txt", "requirements/requirements-schemaregistry.txt"] }
optional-dependencies.protobuf = { file = ["requirements/requirements-protobuf.txt", "requirements/requirements-schemaregistry.txt"] }
optional-dependencies.dev = { file = ["requirements/requirements-tests.txt", "requirements/requirements-docs.txt", "requirements/requirements-examples.txt"] }
optional-dependencies.docs = { file = ["requirements/requirements-docs.txt"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

For generating docs Sphinx needs also the SR dependencies.

Suggested change
optional-dependencies.docs = { file = ["requirements/requirements-docs.txt"] }
optional-dependencies.docs = { file = [
"requirements/requirements-docs.txt",
"requirements/requirements-schemaregistry.txt",
"requirements/requirements-avro.txt",
"requirements/requirements-json.txt",
"requirements/requirements-protobuf.txt"] }

Copy link
Contributor

Choose a reason for hiding this comment

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

Add an all optional dependency

optional-dependencies.all = { file = [
    "requirements/requirements-soaktest.txt",
    "requirements/requirements-docs.txt",
    "requirements/requirements-examples.txt",
    "requirements/requirements-tests.txt",
    "requirements/requirements-schemaregistry.txt",
    "requirements/requirements-avro.txt",
    "requirements/requirements-json.txt",
    "requirements/requirements-protobuf.txt"] }

pyproject.toml Outdated
optional-dependencies.protobuf = { file = ["requirements/requirements-protobuf.txt", "requirements/requirements-schemaregistry.txt"] }
optional-dependencies.dev = { file = ["requirements/requirements-tests.txt", "requirements/requirements-docs.txt", "requirements/requirements-examples.txt"] }
optional-dependencies.docs = { file = ["requirements/requirements-docs.txt"] }
optional-dependencies.tests = { file = ["requirements/requirements-tests.txt"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

Add the SR dependencies here

Suggested change
optional-dependencies.tests = { file = ["requirements/requirements-tests.txt"] }
optional-dependencies.tests = { file = [
"requirements/requirements-tests.txt",
"requirements/requirements-schemaregistry.txt",
"requirements/requirements-avro.txt",
"requirements/requirements-json.txt",
"requirements/requirements-protobuf.txt"
] }

tox.ini Outdated
@@ -1,5 +1,5 @@
[tox]
envlist = flake8,py37,py38,py39,py310
envlist = flake8,py37,py38,py39,py310,py311,py312
Copy link
Contributor

Choose a reason for hiding this comment

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

Below instead of installing
pip install . .[avro] .[schema-registry] .[json] .[protobuf]
we can install
pip install .[tests]

Copy link
Contributor

Choose a reason for hiding this comment

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

deps can be removed

deps =
    # https://docs.pytest.org/en/latest/changelog.html#id53
    -rrequirements/requirement-dev.txt

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add ,tmp-KafkaCluster/* to the flake8 exclusion list, folder created by trivup

Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of pip install .[tests], I have added another block for src and added pip install .[src] instead as there are some extra dependencies in tests which are not main dependencies. Refer pyproject.toml.

Copy link
Contributor

Choose a reason for hiding this comment

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

But here it's a test so it's correct to use tests, if you don't it says that pytest isn't found (you need to remove the .tox folder first).

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also remove

setenv =
    CPPFLAGS=-I{toxinidir}/tmp-build/include
    LDFLAGS=-L{toxinidir}/tmp-build/lib
    C_INCLUDE_PATH={toxinidir}/tmp-build/include
    LD_LIBRARY_PATH={toxinidir}/tmp-build/lib

It overwrites the env variables, for example I've set C_INCLUDE_PATH and LD_LIBRARY_PATH so it uses librdkafka folder instead of installing with make install every time. And this way it doesn't use the defined variables.

Copy link
Contributor

@emasab emasab left a comment

Choose a reason for hiding this comment

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

LGTM, Thank Pranav!

@pranavrth pranavrth merged commit 42f4487 into master Oct 18, 2024
3 checks passed
@pranavrth pranavrth deleted the dev_pyproject-toml branch October 18, 2024 06:45
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.

2 participants