feat: schemas for pluggable deployment service#11979
Merged
Merged
Conversation
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces a comprehensive deployment service framework for LFX, establishing base abstractions, provider-agnostic protocols, custom exception hierarchy, and extensive Pydantic-based schema definitions for deployments, executions, configurations, and snapshots. Comprehensive unit tests validate schema behaviors and constraints. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
Frontend Unit Test Coverage ReportCoverage Summary
Unit Test Results
|
github-actions
Bot
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lfx/src/lfx/services/interfaces.py (1)
34-39:⚠️ Potential issue | 🟡 MinorTypo in docstring: "Auhtenticated" should be "Authenticated".
📝 Proposed fix
class AuthUserProtocol(Protocol): - """Auhtenticated user object (id, username, is_active, is_superuser). + """Authenticated user object (id, username, is_active, is_superuser). Implementations may use User or UserRead from the database layer; this protocol describes the surface needed by consumers of the auth service. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lfx/src/lfx/services/interfaces.py` around lines 34 - 39, Fix the typo in the docstring of AuthUserProtocol: change "Auhtenticated" to "Authenticated" in the class AuthUserProtocol's docstring so it reads "Authenticated user object (id, username, is_active, is_superuser)."; update only the docstring text in the AuthUserProtocol declaration.
🧹 Nitpick comments (5)
src/lfx/src/lfx/services/deployment/base.py (2)
44-52: Abstract property with default return value is unreachable code.The
nameproperty is marked@abstractmethodso subclasses must override it, makingreturn "deployment_service"unreachable. Consider removing the return statement or using...for consistency.♻️ Proposed fix
`@property` `@abstractmethod` def name(self) -> str: """Service name identifier. Returns: str: The service name. """ - return "deployment_service" + ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lfx/src/lfx/services/deployment/base.py` around lines 44 - 52, The abstract property name() in the deployment base (the `@property` `@abstractmethod` def name(self) -> str) includes an unreachable return "deployment_service"; remove the concrete return and make the method abstract-only (replace the body with "..."/pass or raise NotImplementedError) so subclasses must implement it, ensuring the `@abstractmethod` contract is honored; update any docstring as needed but do not provide a default return value in the abstract def name.
64-72: Inconsistent abstract method body style.
list_typesuses...as the body (Line 72) while other abstract methods rely on implicit pass after the docstring. Consider using a consistent style across all abstract methods for readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lfx/src/lfx/services/deployment/base.py` around lines 64 - 72, The abstract method list_types currently uses an explicit ellipsis as its body; make it consistent with the other abstract methods by removing the ellipsis and leaving the method with just the docstring (i.e., implicit pass after the docstring) in the abstract class where list_types is defined so that the method signature and `@abstractmethod` decorator remain but the body style matches the others.src/lfx/tests/unit/services/deployment/test_deployment_schema.py (1)
140-142: Consider adding a test forSnapshotItems(raw_payloads=None).The schema allows
raw_payloadsto beNone, but the current tests only cover the required field case and empty list rejection. Adding a test confirmingSnapshotItems(raw_payloads=None)is valid would improve coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lfx/tests/unit/services/deployment/test_deployment_schema.py` around lines 140 - 142, Add a unit test that verifies SnapshotItems accepts raw_payloads=None: create a new test (e.g., test_snapshot_items_accepts_none_raw_payload) that constructs SnapshotItems(raw_payloads=None) and asserts no ValidationError is raised (optionally assert the instance.raw_payloads is None). This complements the existing tests that cover required field and empty list rejection and references the SnapshotItems class and raw_payloads field.src/lfx/src/lfx/services/deployment/service.py (1)
46-49: Concrete__init__with abstract methods creates an unusual pattern.The class can be instantiated (since
__init__is not abstract), but all other methods will raiseNotImplementedError. This is a valid "template" pattern, but it differs from typical ABC usage where instantiation would fail. If this is intentional (e.g., to allow partial implementations or testing), consider adding a clarifying comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lfx/src/lfx/services/deployment/service.py` around lines 46 - 49, The concrete __init__ calls super().__init__() and self.set_ready() while other methods in this class raise NotImplementedError, creating a template pattern that allows instantiation; either make the initializer abstract to prevent instantiation or keep it concrete but add a clarifying comment that this class is intentionally instantiable for partial implementations/tests. Update the class around __init__ and set_ready to reflect the chosen approach: if preventing instantiation, mark __init__ as abstract (or the class as abstract) so instantiation fails; if allowing it, add a clear docstring/comment on __init__ stating that concrete __init__ + abstract methods is intentional and why.src/lfx/src/lfx/services/deployment/schema.py (1)
27-42: Note: TODO comment at Line 42.The TODO to validate presence of nodes and edges in
datais tracked. Consider creating an issue to implement this validation if it's required for data integrity.Would you like me to open an issue to track implementing the nodes/edges validation in
BaseFlowArtifact.data?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lfx/src/lfx/services/deployment/schema.py` around lines 27 - 42, Add a Pydantic validator on BaseFlowArtifact to enforce that the data field contains required keys "nodes" and "edges" (and that they are lists) so malformed flow payloads are rejected; implement this by adding a `@field_validator`("data") method (or `@model_validator` if you prefer whole-model validation) on the BaseFlowArtifact class that checks data is a dict, verifies "nodes" and "edges" exist and are list-like, and raises a ValueError with a clear message when the check fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lfx/src/lfx/services/interfaces.py`:
- Around line 34-39: Fix the typo in the docstring of AuthUserProtocol: change
"Auhtenticated" to "Authenticated" in the class AuthUserProtocol's docstring so
it reads "Authenticated user object (id, username, is_active, is_superuser).";
update only the docstring text in the AuthUserProtocol declaration.
---
Nitpick comments:
In `@src/lfx/src/lfx/services/deployment/base.py`:
- Around line 44-52: The abstract property name() in the deployment base (the
`@property` `@abstractmethod` def name(self) -> str) includes an unreachable return
"deployment_service"; remove the concrete return and make the method
abstract-only (replace the body with "..."/pass or raise NotImplementedError) so
subclasses must implement it, ensuring the `@abstractmethod` contract is honored;
update any docstring as needed but do not provide a default return value in the
abstract def name.
- Around line 64-72: The abstract method list_types currently uses an explicit
ellipsis as its body; make it consistent with the other abstract methods by
removing the ellipsis and leaving the method with just the docstring (i.e.,
implicit pass after the docstring) in the abstract class where list_types is
defined so that the method signature and `@abstractmethod` decorator remain but
the body style matches the others.
In `@src/lfx/src/lfx/services/deployment/schema.py`:
- Around line 27-42: Add a Pydantic validator on BaseFlowArtifact to enforce
that the data field contains required keys "nodes" and "edges" (and that they
are lists) so malformed flow payloads are rejected; implement this by adding a
`@field_validator`("data") method (or `@model_validator` if you prefer whole-model
validation) on the BaseFlowArtifact class that checks data is a dict, verifies
"nodes" and "edges" exist and are list-like, and raises a ValueError with a
clear message when the check fails.
In `@src/lfx/src/lfx/services/deployment/service.py`:
- Around line 46-49: The concrete __init__ calls super().__init__() and
self.set_ready() while other methods in this class raise NotImplementedError,
creating a template pattern that allows instantiation; either make the
initializer abstract to prevent instantiation or keep it concrete but add a
clarifying comment that this class is intentionally instantiable for partial
implementations/tests. Update the class around __init__ and set_ready to reflect
the chosen approach: if preventing instantiation, mark __init__ as abstract (or
the class as abstract) so instantiation fails; if allowing it, add a clear
docstring/comment on __init__ stating that concrete __init__ + abstract methods
is intentional and why.
In `@src/lfx/tests/unit/services/deployment/test_deployment_schema.py`:
- Around line 140-142: Add a unit test that verifies SnapshotItems accepts
raw_payloads=None: create a new test (e.g.,
test_snapshot_items_accepts_none_raw_payload) that constructs
SnapshotItems(raw_payloads=None) and asserts no ValidationError is raised
(optionally assert the instance.raw_payloads is None). This complements the
existing tests that cover required field and empty list rejection and references
the SnapshotItems class and raw_payloads field.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/lfx/src/lfx/services/deployment/__init__.pysrc/lfx/src/lfx/services/deployment/base.pysrc/lfx/src/lfx/services/deployment/exceptions.pysrc/lfx/src/lfx/services/deployment/schema.pysrc/lfx/src/lfx/services/deployment/service.pysrc/lfx/src/lfx/services/interfaces.pysrc/lfx/tests/unit/services/deployment/__init__.pysrc/lfx/tests/unit/services/deployment/test_deployment_schema.py
github-actions
Bot
added
enhancement
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11979 +/- ##
==========================================
+ Coverage 36.40% 37.27% +0.86%
==========================================
Files 1570 1592 +22
Lines 76655 78263 +1608
Branches 11629 11821 +192
==========================================
+ Hits 27910 29176 +1266
- Misses 47169 47462 +293
- Partials 1576 1625 +49
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
github-actions
Bot
added
enhancement
github-actions
Bot
added
enhancement
github-actions
Bot
added
enhancement
github-actions
Bot
added
enhancement
… exceptions
- Add redeploy, duplicate, create_execution, get_execution to DeploymentServiceProtocol so it matches BaseDeploymentService
- Add @runtime_checkable to DeploymentServiceProtocol
- Use IdLike alias in base.py, service.py, and interfaces.py instead of
raw UUID | str
- Make DeploymentError.error_code required (all subclasses already provide one)
- Store deployment_id on DeploymentNotFoundError regardless of custom message
- Add isinstance checks for nodes/edges in BaseFlowArtifact.validate_data
- Add min_length=1 to BaseFlowArtifact.name
- Remove redundant provider_data re-declaration from DeploymentStatusResult
- Add descriptive messages to all NotImplementedError raises in stub service
- Remove misleading set_ready() from stub DeploymentService
- Export BaseDeploymentService and DeploymentError from package __init__
- Fix "Auhtenticated" typo in AuthUserProtocol docstring
- Parametrize stub method tests to cover all 11 methods
- Add tests for protocol conformance, get_deployment_create_schema,
DeploymentNotFoundError.deployment_id, flow name validation, nodes/edges
type validation, ConfigItem/DeploymentUpdate happy paths
github-actions
Bot
added
lgtm
github-actions
Bot
added
enhancement
HimavarshaVS
pushed a commit
that referenced
this pull request
Mar 10, 2026
* checkout deployment serice schemas
* simplify schemas
* internal facing schema improvements and structuring
* add tests
* use string for name property
* address coderabbit suggestions for implementing todos and adding tests
* address claude maximus comments
* remove teardown from base.add tests for uncovered exception classes
* stub db as AsyncSession
* address review findings: align protocol with ABC, tighten schemas and exceptions
- Add redeploy, duplicate, create_execution, get_execution to DeploymentServiceProtocol so it matches BaseDeploymentService
- Add @runtime_checkable to DeploymentServiceProtocol
- Use IdLike alias in base.py, service.py, and interfaces.py instead of
raw UUID | str
- Make DeploymentError.error_code required (all subclasses already provide one)
- Store deployment_id on DeploymentNotFoundError regardless of custom message
- Add isinstance checks for nodes/edges in BaseFlowArtifact.validate_data
- Add min_length=1 to BaseFlowArtifact.name
- Remove redundant provider_data re-declaration from DeploymentStatusResult
- Add descriptive messages to all NotImplementedError raises in stub service
- Remove misleading set_ready() from stub DeploymentService
- Export BaseDeploymentService and DeploymentError from package __init__
- Fix "Auhtenticated" typo in AuthUserProtocol docstring
- Parametrize stub method tests to cover all 11 methods
- Add tests for protocol conformance, get_deployment_create_schema,
DeploymentNotFoundError.deployment_id, flow name validation, nodes/edges
type validation, ConfigItem/DeploymentUpdate happy paths
* Add domain-specific exception type
* [autofix.ci] apply automated fixes
---------
Co-authored-by: Jordan Frazier <jordan.frazier@datastax.com>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This was referenced Mar 11, 2026
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
2 participants
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.
Introduces the schemas and interface for the pluggable deployment service. Does not register the service. Includes tests for schema validation.
Summary by CodeRabbit
Release Notes
New Features
Tests