Make pydocket optional and unify DI systems#2835
Conversation
Move pydocket to fastmcp[tasks] optional extra. Vendor minimal DI engine for Depends() functionality without full docket. Transform ctx: Context annotations to CurrentContext() defaults at registration time, eliminating the legacy injection codepath.
|
Warning Rate limit exceeded@jlowin has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 13 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (6)
WalkthroughThis PR introduces a vendored, minimal dependency injection engine in Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/fastmcp/server/tasks/requests.py (1)
38-49: Consider making the state mapping a module-level constant instead of a function.The
_get_docket_to_mcp_state_map()function creates a new dictionary on every call (lines 159, 265, and also imported/used insubscriptions.pylines 105, 179). Since the mapping is static and never changes, recreating it repeatedly is inefficient.♻️ Refactor to module-level constant
-def _get_docket_to_mcp_state_map() -> dict[ExecutionState, str]: - """Get the mapping from Docket execution states to MCP task status strings.""" - # Map Docket execution states to MCP task status strings - # Per SEP-1686 final spec (line 381): tasks MUST begin in "working" status - return { - ExecutionState.SCHEDULED: "working", # Initial state per spec - ExecutionState.QUEUED: "working", # Initial state per spec - ExecutionState.RUNNING: "working", - ExecutionState.COMPLETED: "completed", - ExecutionState.FAILED: "failed", - ExecutionState.CANCELLED: "cancelled", - } +# Map Docket execution states to MCP task status strings +# Per SEP-1686 final spec (line 381): tasks MUST begin in "working" status +_DOCKET_TO_MCP_STATE: dict[ExecutionState, str] = { + ExecutionState.SCHEDULED: "working", # Initial state per spec + ExecutionState.QUEUED: "working", # Initial state per spec + ExecutionState.RUNNING: "working", + ExecutionState.COMPLETED: "completed", + ExecutionState.FAILED: "failed", + ExecutionState.CANCELLED: "cancelled", +}Then replace
state_map = _get_docket_to_mcp_state_map()with_DOCKET_TO_MCP_STATEat usage sites (lines 159, 265).src/fastmcp/server/tasks/subscriptions.py (1)
18-19: Consider relocating shared state mapping to avoid cross-module private function import.Importing the private function
_get_docket_to_mcp_state_map()fromrequests.pycreates tight coupling between these modules. When functionality is shared across multiple modules, it's better practice to either:
- Move it to a shared utility module (e.g.,
tasks/common.pyortasks/state.py)- Make it a module-level constant (as suggested in the
requests.pyreview)If you adopt the module-level constant approach suggested for
requests.py, this import would become:from fastmcp.server.tasks.requests import _DOCKET_TO_MCP_STATEAnd usage at lines 105-106 and 179-180 would simply reference
_DOCKET_TO_MCP_STATEdirectly instead of calling a function.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
pyproject.tomlis excluded by none and included by nonetests/server/test_dependencies.pyis excluded by none and included by noneuv.lockis excluded by!**/*.lockand included by none
📒 Files selected for processing (14)
src/fastmcp/_vendor/__init__.pysrc/fastmcp/_vendor/docket_di/README.mdsrc/fastmcp/_vendor/docket_di/__init__.pysrc/fastmcp/dependencies.pysrc/fastmcp/prompts/prompt.pysrc/fastmcp/resources/resource.pysrc/fastmcp/resources/template.pysrc/fastmcp/server/dependencies.pysrc/fastmcp/server/server.pysrc/fastmcp/server/tasks/capabilities.pysrc/fastmcp/server/tasks/config.pysrc/fastmcp/server/tasks/requests.pysrc/fastmcp/server/tasks/subscriptions.pysrc/fastmcp/tools/tool.py
🧰 Additional context used
📓 Path-based instructions (1)
src/fastmcp/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/fastmcp/**/*.py: Python ≥ 3.10 with full type annotations required
Prioritize readable, understandable code - clarity over cleverness. Avoid obfuscated or confusing patterns even if shorter
Follow existing patterns and maintain consistency in code implementation
Be intentional about re-exports - don't blindly re-export everything to parent namespaces. Core types defining a module's purpose should be exported. Specialized features can live in submodules. Only re-export to fastmcp.* for most fundamental types
Never use bare except - be specific with exception types
Files:
src/fastmcp/_vendor/__init__.pysrc/fastmcp/server/tasks/config.pysrc/fastmcp/prompts/prompt.pysrc/fastmcp/server/tasks/subscriptions.pysrc/fastmcp/_vendor/docket_di/__init__.pysrc/fastmcp/server/server.pysrc/fastmcp/dependencies.pysrc/fastmcp/resources/resource.pysrc/fastmcp/server/tasks/capabilities.pysrc/fastmcp/tools/tool.pysrc/fastmcp/resources/template.pysrc/fastmcp/server/dependencies.pysrc/fastmcp/server/tasks/requests.py
🧠 Learnings (5)
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Be intentional about re-exports - don't blindly re-export everything to parent namespaces. Core types defining a module's purpose should be exported. Specialized features can live in submodules. Only re-export to fastmcp.* for most fundamental types
Applied to files:
src/fastmcp/_vendor/__init__.pysrc/fastmcp/dependencies.pysrc/fastmcp/server/dependencies.py
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Python ≥ 3.10 with full type annotations required
Applied to files:
src/fastmcp/_vendor/__init__.pysrc/fastmcp/prompts/prompt.pysrc/fastmcp/server/server.pysrc/fastmcp/resources/resource.pysrc/fastmcp/tools/tool.pysrc/fastmcp/resources/template.pysrc/fastmcp/server/dependencies.pysrc/fastmcp/server/tasks/requests.py
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Follow existing patterns and maintain consistency in code implementation
Applied to files:
src/fastmcp/_vendor/__init__.pysrc/fastmcp/server/dependencies.py
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to tests/**/*.py : Pass FastMCP servers directly to clients for testing without network complexity; only use HTTP transport when explicitly testing network features
Applied to files:
src/fastmcp/_vendor/__init__.pysrc/fastmcp/server/dependencies.py
📚 Learning: 2025-12-25T15:53:07.656Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T15:53:07.656Z
Learning: Applies to src/fastmcp/**/*.py : Never use bare except - be specific with exception types
Applied to files:
src/fastmcp/server/dependencies.py
🧬 Code graph analysis (8)
src/fastmcp/prompts/prompt.py (1)
src/fastmcp/server/dependencies.py (2)
transform_context_annotations(131-247)without_injected_parameters(433-491)
src/fastmcp/server/tasks/subscriptions.py (4)
src/fastmcp/server/context.py (1)
fastmcp(169-174)src/fastmcp/server/low_level.py (2)
fastmcp(45-50)fastmcp(146-151)src/fastmcp/server/tasks/keys.py (1)
parse_task_key(47-75)src/fastmcp/server/tasks/requests.py (1)
_get_docket_to_mcp_state_map(38-49)
src/fastmcp/server/server.py (3)
src/fastmcp/server/context.py (1)
fastmcp(169-174)src/fastmcp/server/low_level.py (2)
fastmcp(45-50)fastmcp(146-151)src/fastmcp/server/dependencies.py (1)
is_docket_available(76-86)
src/fastmcp/dependencies.py (2)
src/fastmcp/_vendor/docket_di/__init__.py (1)
Depends(135-154)src/fastmcp/server/dependencies.py (6)
CurrentContext(624-646)CurrentDocket(666-690)CurrentFastMCP(752-774)CurrentWorker(710-733)Progress(867-903)ProgressLike(781-813)
src/fastmcp/resources/resource.py (1)
src/fastmcp/server/dependencies.py (1)
transform_context_annotations(131-247)
src/fastmcp/resources/template.py (1)
src/fastmcp/server/dependencies.py (2)
transform_context_annotations(131-247)without_injected_parameters(433-491)
src/fastmcp/server/dependencies.py (2)
src/fastmcp/utilities/types.py (2)
find_kwarg_by_type(152-175)is_class_member_of_type(130-149)src/fastmcp/_vendor/docket_di/__init__.py (3)
Dependency(48-61)_Depends(88-132)get_dependency_parameters(69-85)
src/fastmcp/server/tasks/requests.py (3)
src/fastmcp/server/context.py (1)
fastmcp(169-174)src/fastmcp/server/low_level.py (2)
fastmcp(45-50)fastmcp(146-151)src/fastmcp/server/tasks/keys.py (1)
parse_task_key(47-75)
🪛 Ruff (0.14.10)
src/fastmcp/server/dependencies.py
81-81: Unused noqa directive (non-enabled: F401)
Remove unused noqa directive
(RUF100)
97-101: Avoid specifying long messages outside the exception class
(TRY003)
161-161: Do not catch blind exception: Exception
(BLE001)
288-288: Avoid specifying long messages outside the exception class
(TRY003)
303-303: Avoid specifying long messages outside the exception class
(TRY003)
306-306: Avoid specifying long messages outside the exception class
(TRY003)
326-326: Avoid specifying long messages outside the exception class
(TRY003)
361-361: Avoid specifying long messages outside the exception class
(TRY003)
370-370: Consider moving this statement to an else block
(TRY300)
423-426: Avoid specifying long messages outside the exception class
(TRY003)
656-659: Avoid specifying long messages outside the exception class
(TRY003)
700-703: Avoid specifying long messages outside the exception class
(TRY003)
742-742: Avoid specifying long messages outside the exception class
(TRY003)
745-745: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run tests: Python 3.13 on ubuntu-latest
- GitHub Check: Run tests: Python 3.10 on ubuntu-latest
- GitHub Check: Run tests: Python 3.10 on windows-latest
- GitHub Check: Run tests with lowest-direct dependencies
- GitHub Check: Analyze (python)
🔇 Additional comments (35)
src/fastmcp/server/tasks/requests.py (1)
26-32: LGTM: Import organization improved.Moving these imports to the module level (from in-function imports) follows Python best practices and improves code organization.
src/fastmcp/_vendor/__init__.py (1)
1-1: LGTM!Clear documentation for the vendored package marker. The docstring appropriately identifies the purpose of this directory.
src/fastmcp/prompts/prompt.py (2)
30-33: LGTM!Import addition is well-organized and brings in the necessary function for unified DI handling.
448-449: Excellent implementation of unified DI.The transform_context_annotations call is correctly placed after validation and unwrapping, before the DI wrapper. This ensures Context-typed parameters are converted to Depends()-based injection, unifying both DI patterns as intended.
src/fastmcp/resources/resource.py (2)
31-34: LGTM!Import addition mirrors the pattern in prompts.py and correctly brings in the unified DI infrastructure.
450-451: Consistent DI unification pattern.The transform_context_annotations application matches the pattern in prompts.py, ensuring resources handle Context-typed parameters the same way. This consistency is excellent for maintainability.
src/fastmcp/_vendor/docket_di/README.md (1)
1-7: Clear and comprehensive documentation.The README effectively explains the purpose, usage conditions, and lifecycle of the vendored DI engine. The note about isinstance compatibility and the temporary nature of this solution provides good context for future maintainers.
src/fastmcp/server/tasks/capabilities.py (2)
7-10: Correct implementation of runtime package detection.Using
find_specis the appropriate way to check for optional dependencies at runtime. The local function avoids circular imports while providing clean availability checking.
12-34: Well-implemented graceful degradation.The early return of an empty dict when pydocket is unavailable ensures clients won't see task capabilities advertised, providing clean fallback behavior. The updated docstring appropriately documents this conditional behavior.
src/fastmcp/resources/template.py (2)
25-28: LGTM!The import addition is correct and aligns with the DI unification pattern used across other component types (tools, prompts, resources).
541-543: LGTM!The
transform_context_annotations(fn)call is correctly placed after unwrapping callable classes and staticmethods, and beforewithout_injected_parameters(fn). This ensures Context-typed parameters are converted toDepends()-style injections before schema generation, maintaining consistency with the tool and prompt handling paths.src/fastmcp/dependencies.py (2)
6-16: LGTM!The guarded import pattern correctly prioritizes docket's
Dependsfor isinstance compatibility when installed, while falling back to the vendored implementation when docket is unavailable. The docstring clearly documents which features work without the tasks extra.
25-36: LGTM!Adding
ProgressLiketo the public exports is appropriate as it's a core type defining the progress tracking interface. This aligns with the coding guidelines about being intentional with re-exports.src/fastmcp/server/tasks/config.py (2)
94-119: LGTM!The lazy import of
require_docketcorrectly avoids circular dependencies, and the ImportError message clearly guides users to install the tasks extra. The validation flow is well-structured.
133-146: Thestacklevelvalue is incorrect and will not point to the user's decorator usage.The
stacklevel=4does not account for the decorator machinery in actual usage. When@server.tool(task=True)or@tool(task=True)is used, the partial function wrapping adds extra stack frames. The actual stack for@server.tool(task=True)is approximately:
- User code:
@server.tool(task=True)server.tool()at line 1760 (returns partial)- Partial called with function
server.tool()called againLocalProvider.tool()at line 391Tool.from_function()at line 205FunctionTool.from_function()at line 427task_config.validate_function()at line 473warnings.warn()With
stacklevel=4, the warning points toTool.from_function(), not the user's decorator. The correct stacklevel depends on the entry point and may need to be 6+ to reliably point to user code across different calling patterns (@server.tool,@tool, direct calls, etc.).src/fastmcp/tools/tool.py (2)
35-38: LGTM!The import addition is correct and follows the same pattern as other component modules.
617-619: LGTM!The
transform_context_annotations(fn)call is correctly placed after unwrapping callable classes and staticmethods, and before the DI parameter handling. This maintains consistency with the resource template handling and ensures unified DI resolution.src/fastmcp/server/server.py (3)
95-97: LGTM!Moving
Docketimport underTYPE_CHECKINGmaintains type hints for thedocketproperty while avoiding runtime import when docket isn't installed.
460-485: LGTM!The conditional docket initialization is well-structured:
- Server ContextVar is always set first (enabling
CurrentFastMCPto work without docket)- Early return when docket isn't available avoids all task infrastructure setup
- Full task infrastructure (Docket, Worker, component registration) only runs when docket is installed
This ensures graceful degradation while preserving full functionality when the tasks extra is installed.
654-663: LGTM!Skipping task protocol handler registration when docket is unavailable provides clear behavior: clients requesting task operations will receive "method not found" errors, which is appropriate when the feature isn't installed.
src/fastmcp/_vendor/docket_di/__init__.py (4)
1-10: LGTM!Good attribution to the original docket source with MIT license noted.
48-62: LGTM!The
Dependencybase class correctly defines the async context manager interface withsingle: bool = Falseattribute for potential singleton behavior. The__aexit__has a no-op default implementation which is appropriate.
88-132: LGTM!The
_Dependsimplementation correctly:
- Uses class-level ContextVars for per-request caching and exit stack management
- Handles recursive dependency resolution via
_resolve_parameters- Supports all dependency return types: async/sync context managers, coroutines, and regular values
- Caches resolved values to avoid redundant resolution within a single request
135-154: LGTM!The
Dependsfactory function provides a clean API matching docket's interface. The docstring examples clearly demonstrate supported dependency patterns.src/fastmcp/server/dependencies.py (11)
62-68: LGTM!The ContextVars are correctly typed with optional defaults. Using
weakref.reffor the server avoids reference cycles.
76-101: LGTM!The
is_docket_available()function correctly caches the import check result for performance. Therequire_docket()function provides a clear error message with installation instructions. The global caching is appropriate since package availability doesn't change at runtime.
108-125: LGTM!The guarded imports correctly prioritize docket's classes for isinstance compatibility when installed, falling back to the vendored implementation otherwise. The separate try/except for
Progress(lines 121-125) handles the case where docket is installed but might have a different structure.
159-162: BareExceptioncatch is acceptable here.The static analysis flags this, but catching
Exceptionforget_type_hints()is intentional and necessary. This function can raise various exceptions (NameError, AttributeError, RecursionError, etc.) when type hints reference undefined names or have circular dependencies. Falling back to raw annotations is the correct behavior.
130-247: LGTM - well-structured signature transformation.The
transform_context_annotationsfunction correctly:
- Identifies Context-typed parameters that don't already have a Dependency default
- Preserves Python's signature structure by grouping parameters by kind
- Reorders POSITIONAL_OR_KEYWORD parameters to maintain "no-default before default" ordering
- Handles bound methods by preserving the 'self' parameter
- Clears signature caches to ensure downstream code sees the updated signature
The comment at line 193-195 correctly explains why
CurrentContext()is used instead ofDepends(get_context).
250-279: LGTM!The
_clear_signature_cachesfunction correctly clears caches from both the vendored DI engine and docket (when available). The try/except for docket cache clearing handles version differences gracefully.
375-426: LGTM!The
get_access_token()function correctly:
- Prioritizes the HTTP request's scope (more reliable for long-lived connections)
- Falls back to the SDK's context var
- Converts SDK AccessToken types to FastMCP AccessToken with proper error handling
The conversion logic handles optional fields appropriately.
432-491: LGTM!The updated
without_injected_parametersfunction correctly:
- Uses
find_kwarg_by_type(imported from utilities) for Context detection- Gets dependency parameters from the transformed signature
- Creates an async wrapper that handles dependency resolution
- Preserves function metadata for schema generation
780-814: LGTM!The
ProgressLikeprotocol correctly defines the common interface for progress tracking with@runtime_checkablefor isinstance checks. The interface matches bothInMemoryProgressand Docket'sProgress.
816-864: LGTM!The
InMemoryProgressclass correctly implements theProgressLikeprotocol with proper validation (total >= 1, amount >= 1) and atomic-like increment behavior.
867-901: Verify thatDocketProgress.__aenter__()only raisesLookupErrorwhen not in worker context.The code assumes
DocketProgress.__aenter__()raisesLookupErrorspecifically when called outside a worker context. If it can raise other exceptions (e.g., from internal Docket errors), those would propagate uncaught instead of triggering the fallback toInMemoryProgress. Consider either documenting this assumption explicitly or catching a broader exception type that encompasses bothLookupErrorand other potential runtime errors from Docket.
Docket/Worker infrastructure now only spins up when there are task-enabled components (task=True), not just when pydocket is installed. This avoids overhead for users who have pydocket as a transitive dependency but don't use background tasks. - Collect task-enabled components before creating Docket - Skip Docket infrastructure entirely if no task components exist - Progress dependency falls back to InMemoryProgress when Docket isn't running - Updated error messages for CurrentDocket/CurrentWorker to explain requirement - Added documentation about startup requirement for task components
Moves
pydocketfrom core dependencies to an optionalfastmcp[tasks]extra. The base install no longer pulls in distributed task infrastructure—Redis clients, Lua environments, async job machinery—that's irrelevant for simple MCP servers.What changed
pydocket is now optional. A vendored minimal DI engine provides
Depends()functionality without docket. Task features (task=True,Progress(),CurrentDocket()) require the extra and give clear errors if it's missing.Unified DI system. Legacy
ctx: Contexttype annotations are now transformed toctx: Context = CurrentContext()at registration time. One codepath instead of two, and the vendored DI engine only needs to understandDependency.Graceful degradation. Task capabilities return empty
{}when pydocket isn't installed, so clients won't see task support advertised. Attempting to usetask=Truewithout the extra raises:Also adds a warning when combining
task=Truewith Context parameters (Context isn't available in workers).cc @chrisguidry — the vendored DI engine is a minimal subset of docket's dependency injection. Once the DI component is spun out as a standalone library, we'll replace the vendor with a proper dependency.