fix: Resolve ruff check failures in lfx deployment service#11992
Merged
Conversation
…s linting warnings
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 pull request updates the deployment service module with minor changes: a multi-line exception message string, exception type adjustments for validation errors, addition of type-checker suppression comments across method parameters, a new teardown method, and test syntax updates for exception raising. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (5 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
|
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (42.29%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #11992 +/- ##
==========================================
- Coverage 37.33% 37.31% -0.02%
==========================================
Files 1592 1592
Lines 78279 78279
Branches 11824 11824
==========================================
- Hits 29226 29211 -15
- Misses 47432 47448 +16
+ Partials 1621 1620 -1
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
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lfx/src/lfx/services/deployment/schema.py`:
- Around line 60-63: Change the exceptions raised in the flow validation to
ValueError so Pydantic v2 wraps them into a ValidationError: in the field
validation block that checks value["nodes"] and value["edges"] (the validator
that raises "Flow 'nodes' must be a list" and "Flow 'edges' must be a list"),
replace the TypeError(...) raises with ValueError(...) so Pydantic will
aggregate these as validation errors rather than allowing TypeError to
propagate.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lfx/src/lfx/services/deployment/exceptions.pysrc/lfx/src/lfx/services/deployment/schema.pysrc/lfx/src/lfx/services/deployment/service.pysrc/lfx/tests/unit/services/deployment/test_deployment_exceptions.py
Comment on lines
+60
to
+63
| raise TypeError(msg) | ||
| if not isinstance(value["edges"], list): | ||
| msg = "Flow 'edges' must be a list" | ||
| raise ValueError(msg) | ||
| raise TypeError(msg) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Pydantic v2 field_validator TypeError ValidationError conversion behavior
💡 Result:
In Pydantic v2, the key behavior change is:
- Only “validation-style” exceptions raised inside validators—
ValueError,AssertionError, orPydanticCustomError—are captured and aggregated into aValidationError. (docs.pydantic.dev) TypeError(and most other exceptions) raised inside a@field_validatoris not converted into aValidationErroranymore; it bubbles up as the originalTypeError. (docs.pydantic.dev)
What this means in practice
- If your validator code accidentally triggers a
TypeError(e.g., calling a function with the wrong types likestr.lower(v)whenvis anint, or using a wrong validator signature), your model construction/model_validate()will raiseTypeErrordirectly, notValidationError. (docs.pydantic.dev) - If you want the error to appear as a field error inside a
ValidationError, raiseValueError(orPydanticCustomError) instead ofTypeError. (docs.pydantic.dev)
Citations:
- 1: https://docs.pydantic.dev/2.5/concepts/validators/?utm_source=openai
- 2: https://docs.pydantic.dev/2.0/migration/?utm_source=openai
- 3: https://docs.pydantic.dev/2.0/migration/?utm_source=openai
- 4: https://docs.pydantic.dev/2.5/concepts/validators/?utm_source=openai
Use ValueError instead of TypeError to preserve Pydantic validation semantics.
At Line 60 and Line 63, raising TypeError in a @field_validator bypasses Pydantic v2's ValidationError wrapping. In Pydantic v2, only ValueError, AssertionError, and PydanticCustomError are converted into a ValidationError; TypeError propagates directly, breaking normal error aggregation in request validation paths.
Proposed fix
if not isinstance(value["nodes"], list):
msg = "Flow 'nodes' must be a list"
- raise TypeError(msg)
+ raise ValueError(msg) # noqa: TRY004 - preserve ValidationError behavior
if not isinstance(value["edges"], list):
msg = "Flow 'edges' must be a list"
- raise TypeError(msg)
+ raise ValueError(msg) # noqa: TRY004 - preserve ValidationError behavior🤖 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 60 - 63, Change
the exceptions raised in the flow validation to ValueError so Pydantic v2 wraps
them into a ValidationError: in the field validation block that checks
value["nodes"] and value["edges"] (the validator that raises "Flow 'nodes' must
be a list" and "Flow 'edges' must be a list"), replace the TypeError(...) raises
with ValueError(...) so Pydantic will aggregate these as validation errors
rather than allowing TypeError to propagate.
viktoravelino
approved these changes
Mar 3, 2026
HimavarshaVS
pushed a commit
that referenced
this pull request
Mar 10, 2026
* fix: Improve message formatting in DeploymentNotConfiguredError * fix: Change ValueError to TypeError for invalid flow nodes and edges * fix: Add noqa comments to suppress ARG002 warnings for unused function arguments * fix: Update exception raising in tests to improve clarity and suppress linting warnings * fix: Change TypeError to ValueError so pydantic recognizes the error
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.
Summary
DeploymentNotConfiguredErrorto stay within 120-char line limit (E501)ValueErrortoTypeErrorforisinstancechecks inBaseFlowArtifact.validate_data(TRY004)# noqa: ARG002to intentionally unused stub method parameters inDeploymentService(ARG002)pytest.raisesblock in tests (RSE102, PT012)Summary by CodeRabbit
Bug Fixes
New Features