Align confluent driver type annotations with the aiokafka driver - #756
Merged
Conversation
The confluent transport driver was the only driver still carrying implicit-Optional parameter defaults and fully unannotated functions, which `mypy -p faust` reported as 38 errors -- every one of them absent from the aiokafka driver next to it. Bring the annotations up to the same standard: * Spell out `Optional[...]` wherever a parameter defaults to `None` (`config`, `retention`, `compacting`, `deleting`, `transactional_id`, `partition`). PEP 484 prohibits implicit Optional, so these were also Liskov violations against the `ConsumerT`/`ProducerT`/`ConsumerThread` supertypes, which already declare the widened types. * Annotate `AsyncConsumer.__init__` and its `close`/`subscribe`/`assign`/ `poll` methods, reusing the `PartitionsRevokedCallback` and `PartitionsAssignedCallback` aliases faust already defines. * Correct `AsyncConsumer.assignment()` to `List[TopicPartition]`, what `confluent_kafka.Consumer.assignment()` actually returns, rather than `Set[TP]`. `ConfluentConsumerThread.assignment()` already funnels it through `ensure_TPset()`. * Return `Mapping[TP, int]` from `earliest_offsets`/`highwaters` and their helpers, matching both the `ConsumerThread` base class and the aiokafka driver. * Type `ProducerThread.produce`'s key/value/partition as optional, which is what the `partition is not None` branch in its body already assumes, and give `on_delivery` the signature confluent calls it with. * Reach the underlying consumer in `ConfluentConsumerThread.key_partition` via `_ensure_consumer()` like every other method in the class, so an unstarted thread raises `ConsumerNotStarted` instead of an `AttributeError` on `None`. No runtime behaviour changes beyond that last point. `mypy -p faust` drops from 552 to 514 errors with no new ones anywhere, and the driver's 57 unit tests still pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012TbXZ1ATm7RvvDsqgZ3Xy6
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #756 +/- ##
=======================================
Coverage 96.06% 96.06%
=======================================
Files 103 103
Lines 11072 11072
Branches 1191 1191
=======================================
Hits 10636 10636
Misses 345 345
Partials 91 91 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The previous commit gave it `Callable[[Optional[BaseException], _Message], None]`, reasoning from ProducerProduceFuture.set_from_on_delivery's own signature. That is not the contract confluent uses: its py.typed stubs declare on_delivery as `Callable[[Optional[KafkaError], Message], None]`, and KafkaError does not derive from BaseException -- KafkaException is the exception type, KafkaError is a plain status object. So set_from_on_delivery's `self.set_exception(err)` gets handed a non-exception and raises TypeError instead of failing the future with the delivery error. The "XXX Not sure what err is here" comment above it turns out to have been well founded. Fixing that is a behaviour change and belongs in its own patch. Until then, annotating the parameter precisely would assert a contract the code does not honour, so leave it as a bare Callable -- which is also what the aiokafka driver uses for its callbacks -- and record why. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012TbXZ1ATm7RvvDsqgZ3Xy6
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
faust/transport/drivers/confluent.pywas the only transport driver still carrying implicit-Optionalparameter defaults and fully unannotated functions — patterns absent fromfaust/transport/drivers/aiokafka.pysitting next to it. This PR brings the confluent driver's annotations up to the same standard, without reworking its logic.What changed
Optional[...]for everyNonedefault —config,retention,compacting,deleting,transactional_id,partition. PEP 484 prohibits implicitOptional, so these were also Liskov violations against theConsumerT/ProducerT/ConsumerThreadsupertypes, which already declare the widened types. AffectsConsumer.create_topic,ConfluentConsumerThread.create_topic,ConfluentConsumerThread.key_partition,Producer.create_topic,Producer.send,Producer.send_and_wait, andTransport._topic_config.AsyncConsumeris now annotated —__init__plusclose/subscribe/assign/poll, reusing thePartitionsRevokedCallbackandPartitionsAssignedCallbackaliases faust already defines infaust.types.transports.AsyncConsumer.assignment()returnsList[TopicPartition], which is whatconfluent_kafka.Consumer.assignment()actually returns, rather thanSet[TP].ConfluentConsumerThread.assignment()already funnels the result throughensure_TPset().earliest_offsets/highwaters(and their helpers) returnMapping[TP, int], matching both theConsumerThreadbase class and the aiokafka driver, instead of needlessly narrowing toMutableMapping.ProducerThread.producetakes optional key/value/partition — which is what theif partition is not Nonebranch in its own body already assumes.on_deliveryis deliberately left as a bareCallable; see below.ConfluentConsumerThread.key_partitionreaches the consumer via_ensure_consumer(), like every other method in the class, so an unstarted thread raisesConsumerNotStartedrather than anAttributeErroronNone. This is the only behaviour change in the PR, and it mirrorsAIOKafkaConsumerThread.key_partition.Effect on mypy
Numbers depend on the mypy version, because 2.x reads inline types from
py.typedpackages (includingconfluent_kafka) that 1.x ignored. Both directions are an improvement:A file-by-file diff of the full error list confirms no other module changes in either case.
The 17 remaining under 2.3.0 are all pre-existing defects, and several become visible only because of this PR: annotating
AsyncConsumer.__init__lets mypy resolveself.consumerto the realconfluent_kafka.Consumerinstead ofAny, so call sites that were previously unchecked now get checked. What it surfaces is genuine —AsyncConsumerdefines neitherseeknorseek_to_beginning, yet_seek_waitandseek_to_beginningcall both; and several sync callables are handed tocall_thread, which expects awaitables.On
on_deliveryAn earlier revision of this branch annotated it
Callable[[Optional[BaseException], _Message], None], reasoning fromProducerProduceFuture.set_from_on_delivery's own signature. That is wrong: confluent's stubs declareCallable[[Optional[KafkaError], Message], None], andKafkaErrordoes not derive fromBaseException—KafkaExceptionis the exception type,KafkaErroris a plain status object. Soset_from_on_delivery'sself.set_exception(err)is handed a non-exception and raisesTypeErrorinstead of failing the future with the delivery error. The# XXX Not sure what err is herecomment above it turns out to have been well founded. Fixing that is a behaviour change and belongs in its own patch, so the parameter is left as a bareCallablewith a comment recording why.Other latent bugs found, not fixed here
Producer.sendcallsProducerThread.produce(topic, value, key, partition)against a(topic, key, value, partition)signature — key and value are swapped.Producer.key_partitioncallsself._producer_thread.producer.list_topics(...), butProducerThread.produceris the faustProducer, which has no such method.Verification
pytest tests/unit/transport/drivers/test_confluent.py— 57 passed (the same suite the dedicated confluent CI leg runs).isort --check,black --check, andflake8clean on the changed file.No issue to link; this is a type-annotation cleanup. Follow-up #757 wires mypy into CI behind a ratchet so this class of regression is caught automatically.