Merge kafka-python - Githubissues

ods commented 8 months ago

Changes

Merges relevant code from kafka-python. Motivations: it's not possible to support new Python versions without changes in kafka-python. See statement from kafka-python's author.

The changes are divided on subtree split & add commits that brings original code without changes and separate commits to adopt those changes into aiokafka. This allows to see changes made into the code when reviewed commit-by-commit.

Checklist

[ ] I think the code is well written
[ ] Unit tests for the changes exist
[ ] Documentation reflects the changes
[ ] Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> (e.g. 588.bugfix)
- if you don't have an issue_id change it to the pr id after creating the PR
- ensure type is one of the following:
- .feature: Signifying a new feature.
- .bugfix: Signifying a bug fix.
- .doc: Signifying a documentation improvement.
- .removal: Signifying a deprecation or removal of public API.
- .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

codecov[bot] commented 8 months ago

Codecov Report

Merging #932 (66e2999) into master (00349a8) will decrease coverage by 13.35%. The diff coverage is 69.49%.

@@             Coverage Diff             @@
##           master     #932       +/-   ##
===========================================
- Coverage   97.56%   84.21%   -13.35%     
===========================================
  Files          30       87       +57     
  Lines        5451    10085     +4634     
===========================================
+ Hits         5318     8493     +3175     
- Misses        133     1592     +1459

Flag	Coverage Δ
cext	`79.26% <69.49%> (-9.11%)`	:arrow_down:
integration	`84.19% <69.49%> (-13.33%)`	:arrow_down:
purepy	`83.96% <69.49%> (-13.14%)`	:arrow_down:
unit	`51.55% <67.34%> (+13.44%)`	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
aiokafka/abc.py	`70.58% <100.00%> (-1.64%)`	:arrow_down:
aiokafka/admin/__init__.py	`100.00% <100.00%> (ø)`
aiokafka/admin/new_partitions.py	`100.00% <100.00%> (ø)`
aiokafka/client.py	`98.16% <100.00%> (-0.01%)`	:arrow_down:
aiokafka/consumer/consumer.py	`97.91% <100.00%> (ø)`
aiokafka/consumer/fetcher.py	`97.48% <100.00%> (ø)`
aiokafka/consumer/group_coordinator.py	`99.17% <100.00%> (ø)`
aiokafka/coordinator/protocol.py	`100.00% <100.00%> (ø)`
aiokafka/errors.py	`100.00% <100.00%> (ø)`
aiokafka/metrics/__init__.py	`100.00% <100.00%> (ø)`
... and 64 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

kPsarakis commented 8 months ago

Could you also add Python 3.12 to the CI?

ods commented 8 months ago

Could you also add Python 3.12 to the CI?

I don't think it's good to mix those changes, PR is already too big

kPsarakis commented 8 months ago

I managed to run the tests with Python 3.11.5 in a conda environment. However, when I created a Python 3.12.0 environment, I got the following error with flake8 while running the tests:

make cov
black --check aiokafka/util.py aiokafka/structs.py setup.py
All done! ✨ 🍰 ✨
3 files would be left unchanged.
flake8 aiokafka tests setup.py
Traceback (most recent call last):
  File "/miniconda3/envs/aiok/bin/flake8", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/miniconda3/envs/aiok/lib/python3.12/site-packages/flake8/main/cli.py", line 22, in main
    app.run(argv)
  File "/miniconda3/envs/aiok/lib/python3.12/site-packages/flake8/main/application.py", line 375, in run
    self._run(argv)
  File "/miniconda3/envs/aiok/lib/python3.12/site-packages/flake8/main/application.py", line 363, in _run
    self.initialize(argv)
  File "/miniconda3/envs/aiok/lib/python3.12/site-packages/flake8/main/application.py", line 343, in initialize
    self.find_plugins(config_finder)
  File "/miniconda3/envs/aiok/lib/python3.12/site-packages/flake8/main/application.py", line 157, in find_plugins
    self.check_plugins = plugin_manager.Checkers(local_plugins.extension)
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/miniconda3/envs/aiok/lib/python3.12/site-packages/flake8/plugins/manager.py", line 363, in __init__
    self.manager = PluginManager(
                   ^^^^^^^^^^^^^^
  File "/miniconda3/envs/aiok/lib/python3.12/site-packages/flake8/plugins/manager.py", line 243, in __init__
    self._load_entrypoint_plugins()
  File "/miniconda3/envs/aiok/lib/python3.12/site-packages/flake8/plugins/manager.py", line 261, in _load_entrypoint_plugins
    eps = importlib_metadata.entry_points().get(self.namespace, ())
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'EntryPoints' object has no attribute 'get'
make: *** [Makefile:22: lint] Error 1

I will investigate it a bit and get back to you.

ods commented 8 months ago

@kPsarakis Thank you for starting working on it. I suggest to concentrate on this merge using Python versions that are used in test matrix. Support for 3.12 is not just adding one version to YAML files, there is bunch of problems to solve:

aiokafka uses setup.py which is deprecated and I already face some problems, like inability to install in edit mode which is vital for local development. We have to move to pyproject.toml build (probably using hatchling) and I expect some difficulties with Cython here.
The pinned versions of libs and tools may not work. You've already stumbled upon the problem with flake, and there will be more I think.
flake8/pylint and black have different opinions on how to format code. It would be nice to replace all linters with ruff + black.
python-snappy is not updated and we may see more problems with it in 3.12. It would be good to replace it with cramjam.
I expect some deprecation warnings that we'll need to fix.

kPsarakis commented 8 months ago

@ods I got confused jumping to this PR from the Python 3.12 compatibility issue. That's why I focused on 3.12. I managed to run the tests with 3.12 by bumping the following requirements in the requirements-ci.txt:

flake8==6.1.0
pytest==7.4.2
pytest-cov==4.1.0
pytest-asyncio==0.21.1

And I was running them with AIOKAFKA_NO_EXTENSIONS=1. I also had to make some changes based on some new flake8 suggestions. But I am stopping for now. After finishing this PR, I could also help with any of the points you raised.

Now, regarding this PR:

I searched for any remaining kafka-python code or anything missing, and it looks good. Also, since all tests are passing, it is relatively safe to merge, given the amount of changes in this PR.

Additionally, we use Kafka as an ingress/egress for a system we are building and aiokafka as the client library. I added git+https://github.com/ods/aiokafka@merge-kafka-python instead of aiokafka==0.8.1 in our requirements (Python 3.11), and our end-to-end tests pass without regression.

ods commented 8 months ago

Additionally, we use Kafka as an ingress/egress for a system we are building and aiokafka as the client library. I added git+https://github.com/ods/aiokafka@merge-kafka-python instead of aiokafka==0.8.1 in our requirements (Python 3.11), and our end-to-end tests pass without regression.

Thanks, testing with real-life scenarios are really valuable for this change!

aio-libs / aiokafka

Merge kafka-python #932

Changes

Checklist

Codecov Report