docs(cluster): say which async ClusterPipeline methods must be awaited - #4264
docs(cluster): say which async ClusterPipeline methods must be awaited#4264ckarnell wants to merge 3 commits into
Conversation
Follow-up invited on redis#4259. The class docstring chains .delete(...) without awaiting, and its note lists UNLINK beside DELETE, but unlink is a coroutine that returns None, so it neither chains nor runs unless awaited. That is the confusion redis#4259 came from. Lists the ten coroutine methods, contrasts staging with awaiting, notes that a transactional block behaves differently again because execute_command returns a response there rather than the pipeline, and fills in the empty discard() docstring and the missing unlink() one. Docs only, no behaviour change.
petyaslavova
left a comment
There was a problem hiding this comment.
Hey @ckarnell, thank you for your contribution! This is a great follow-up, and the unlink and discard docstrings are accurate and useful. Three corrections are needed in the class docstring before we can merge it.
They return None rather than the pipelinedoes not hold for half the list:executereturnsList[Any],initializereturns the pipeline (that is whatawait pipeand__aenter__use),himport_preparereturnsbool, andhimport_discard/himport_discard_allreturnint. Onlyreset,discard,watch,unwatchandunlinkreturnNone.- The
multi()paragraph is inverted.TransactionStrategy._execute_commandonly sends immediately whenself._watching or args[0] in IMMEDIATE_EXECUTE_COMMANDSandnot self._explicit_transaction;multi()sets_explicit_transaction = True, so inside amulti()block commands are queued and do return the pipeline. The immediate, must-be-awaited case is betweenwatch()andmulti()— seetest_transaction_with_watched_keys, whereawait pipe.get("a")is awaited while watching andpipe.set(...)aftermulti()is not. - Please scope the list of ten, since it only covers methods defined on
ClusterPipeline. Inherited coroutines such ascommand_info,cluster_delslots,client_tracking_on/off,hotkeys_*and the*scan_itergenerators are not included, andwatch/unwatch/discardraiseRedisClusterExceptionoutside a transactional pipeline whilehimport_*stage nothing.
Could you also mirror the discard() and unlink() docstrings on the sync ClusterPipeline in redis/cluster.py? The same empty/missing docstrings and the same single-key restriction are there, and we keep the two stacks aligned. Once those points are addressed I am happy to merge this.
…m on the sync class
|
All three were right, fixed in 2c5a4a3. The return types are corrected. The multi() paragraph was backwards, my error. multi() sets _explicit_transaction, and the immediate-send branch requires not self._explicit_transaction, so inside multi() commands queue and do chain, which is what test_transaction_with_watched_keys shows. Rewrote it to say that. I also added the twelve inherited coroutines that sat outside the list, including the four you named and the hotkeys_ family, and mirrored the docstrings onto the sync ClusterPipeline, since sync discard had an empty docstring and unlink had none. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c5a4a322b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| such as ``command_info``, ``cluster_delslots``, ``client_tracking_on`` and the | ||
| ``hotkeys_*`` family must be awaited too and are not listed here. |
There was a problem hiding this comment.
Avoid recommending inherited async commands on pipelines
On an async cluster pipeline this guidance makes commands like client_tracking_on and cluster_delslots look like supported pipeline steps, but those inherited async client methods assume execute_command is awaitable. Here ClusterPipeline.execute_command returns the pipeline, so await pipe.client_tracking_on() awaits ClusterPipeline.__await__ and reinitializes/clears the staged command, while cluster_delslots tries to create tasks from pipeline objects; users following this doc can silently drop queued work or hit TypeError instead of staging a command.
AGENTS.md reference: AGENTS.md:L176-L179
Useful? React with 👍 / 👎.
petyaslavova
left a comment
There was a problem hiding this comment.
Hey @ckarnell, thanks for the quick turnaround - the return types, the WATCH → MULTI example and the sync mirror all check out now. Two things still need a fix before merge.
-
The inherited-coroutines paragraph names the wrong examples. Of the four, only
client_tracking_on/client_tracking_offactually await the pipeline and clear the queue (theyawait self.client_tracking(...), which returns the pipeline).command_inforaisesNotImplementedError(redis/commands/core.py), the clusterhotkeys_*methods raiseNotImplementedError(redis/commands/cluster.py), andcluster_delslotswrapsexecute_commandinasyncio.create_task, which raisesTypeErroron a pipeline object. Please useclient_tracking_on/client_tracking_offfor the discard-the-queue warning and mention the raising methods separately. -
On the sync class the
unlinkdocstring's "unlikedelete" contrast does not hold:ClusterPipeline.deletealso returnsNoneandPipelineStrategy.deleteraises the same multiple-keysRedisClusterException. In sync, neither chains and both are single-key outside a transaction, so please reword it for that stack.
Two optional extras while you are in there: "commands are sent as they are issued" is really "executed when awaited" (TransactionStrategy._execute_command hands back the un-awaited _immediate_execute_command coroutine), and the note above your new section still lists UNLINK as split across nodes even though the pipeline accepts only one key. Fixing those in the same PR would be welcome. Once points 1 and 2 are in, I am happy to merge.
Mukller
left a comment
There was a problem hiding this comment.
Docs-only PR — I fact-checked the new docstring against the code on master, and every claim holds:
- The ten listed methods are coroutines defined on
ClusterPipeline: verified viainspect.iscoroutinefunction+__dict__membership forwatch, unwatch, unlink, discard, reset, execute, initialize, himport_prepare, himport_discard, himport_discard_all— all True on both counts. deletevsunlinkasymmetry is real:deleteis an inherited staging command (not a coroutine, chainable), whileunlinkis a coroutine defined on the class that must be awaited. The example calling this out is the most valuable part of the doc — it's exactly the trap.- "Outside a transactional pipeline watch/unwatch/discard raise RedisClusterException": confirmed — the non-transactional execution strategy raises
RedisClusterException("method ... is not supported outside of transactional context"). - watch→multi immediate-send semantics: consistent with the strategy implementations referenced above.
This documents behavior that today can only be learned by reading three classes' worth of source or by getting bitten at runtime (await pipe.unlink(...) vs pipe.delete(...)). The level of detail is appropriate given how surprising the staging/await split is.
One tiny suggestion, non-blocking: since himport_* "act on the shared fieldset registry rather than staging", they're arguably a separate concern from the await-list of pipeline steps — if you ever revisit, splitting them into their own paragraph would make the pipeline-relevant list even sharper. Fine as-is though.
CI on the PR was green when I checked; no product code touched.
Follow-up you invited on #4259: making it clear which
ClusterPipelinemethods have to be awaited.The confusion is visible in the class docstring itself. The usage example chains
.delete("A", "B", "K").execute()without awaiting thedelete, and the note underneath listsUNLINKright besideDELETEas though the two behave alike. They do not.unlinkis a coroutine that returnsNone, so it neither chains nor stages anything unless you await it.So the docstring now lists the ten coroutine methods, says plainly that they return
Noneinstead of the pipeline, and shows the contrast:While checking the claim I found a third case worth a line: inside a
multi()block,TransactionStrategyoverridesexecute_commandand returns a response instead of the pipeline, so commands do not chain there either. My first draft said staged commands always return the pipeline, which was wrong outside the default strategy.Also filled in two gaps next door:
discard()had an empty docstring, andunlink()had none. Theunlinkone records that outside a transaction it takes a single key and raisesRedisClusterExceptionfor more, which is currently only discoverable by readingPipelineStrategy.Docs only, no behaviour change.
ruff checkandvulture --min-confidence 80are clean.ruff format --checkreports this file, but it does so on an unmodified checkout too, so I left it alone instead of reformatting code I did not touch.The list of ten was generated from the class, not typed by hand, so it includes the
himport_*methods I would otherwise have missed.Note
Low Risk
Documentation-only changes with no runtime or API behavior modifications.
Overview
Async
ClusterPipelinegets a new docstring section that lists the ten coroutine methods that must be awaited individually, which of those returnNone(breaking chaining), and warns that inherited command coroutines are not pipeline steps—awaiting them re-initializes the pipeline.It also documents the
unlinkvsdeletecontrast (delete chains; asyncunlinkmust be awaited), WATCH → MULTI behavior (commands between them run immediately and must be awaited; aftermulti()staging chains again), and notes on non-transactionalwatch/unwatch/discardandhimport_*registry behavior.discard()andunlink()docstrings are filled in on both async (redis/asyncio/cluster.py) and sync (redis/cluster.py) cluster pipelines, including the single-key-onlyunlinkrule outside transactions.Reviewed by Cursor Bugbot for commit 2c81c47. Bugbot is set up for automated code reviews on this repo. Configure here.