refactor: mount data blueprint via WSGI and adopt Pydantic in engine blueprint#1179
refactor: mount data blueprint via WSGI and adopt Pydantic in engine blueprint#1179koladefaj wants to merge 4 commits intoinclusionAI:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the InferenceDataProxy by mounting a centralized Flask blueprint for RTensor data storage endpoints, effectively removing duplicate logic. Additionally, it introduces Pydantic models in engine_blueprint.py to standardize request validation for the engine API. A critical typo was found in the set_env endpoint where an incomplete attribute name is used, which will cause a runtime error.
|
Hi @garrett4wade, I've mounted the |
Review Findings1. Empty-string validation regressed on engine RPC inputsFile: The old code used class CreateEngineRequest(BaseModel):
engine: str # accepts ""
engine_name: str # accepts ""
class CallEngineRequest(BaseModel):
method: str # accepts ""
engine_name: str # accepts ""Consequences:
Suggested fix — enforce non-empty strings: from pydantic import StringConstraints
from typing import Annotated
NonEmptyStr = Annotated[str, StringConstraints(min_length=1)]
class CreateEngineRequest(BaseModel):
engine: NonEmptyStr
engine_name: NonEmptyStr
init_args: list[Any] = []
init_kwargs: dict[str, Any] = {}
class CallEngineRequest(BaseModel):
method: NonEmptyStr
engine_name: NonEmptyStr
args: list[Any] = []
kwargs: dict[str, Any] = {}
rpc_meta: dict[str, Any] | None = None2. No unit tests for Pydantic model validation edge casesSeverity: Medium The PR adds 3 Pydantic models ( Suggested additions — cover at minimum: def test_create_engine_empty_string():
resp = client.post("/create_engine", json={"engine": "", "engine_name": "test"})
assert resp.status_code == 400
def test_create_engine_missing_fields():
resp = client.post("/create_engine", json={})
assert resp.status_code == 400
def test_call_engine_missing_method():
resp = client.post("/call", json={"engine_name": "actor/0"})
assert resp.status_code == 400
def test_set_env_invalid_json():
resp = client.post("/set_env", data="not json", content_type="application/json")
assert resp.status_code == 400 |
|
I've addressed the review findings:
Ready for review @garrett4wade |
Description
This PR refactors the experimental inference service and the core infrastructure blueprints to eliminate code duplication and standardize request validation.
Key Changes:
WSGI Mounting: Mounted the legacy Flask
data_bpblueprint into the FastAPI data proxy app usingWSGIMiddleware. This removes ~60 lines of duplicate endpoint logic inareal/experimental/inference_service/data_proxy/app.py.Pydantic Migration (Engine BP): Extended the Pydantic pattern to
areal/infra/rpc/guard/engine_blueprint.py. AddedSetEnvRequest,CreateEngineRequest, andCallEngineRequestmodels to replace manual JSON parsing and dictionary lookups, improving type safety and error handling.Consistency: Updated comments in the data proxy to clarify the new mounted architecture and the source of truth for
RTensorstorage logic.Related Issue
Fixes #1106 (Follow-up to #1154)
Type of Change
Checklist
pre-commit run --all-files)./docs/build_all.sh)main/review-prcommand/create-prBreaking Change Details (if applicable): N/A
Additional Context
Note on Documentation: Because the /data/ endpoints are now handled by the WSGI middleware layer, they will no longer appear in the auto-generated FastAPI Swagger UI (/docs). This trade-off was chosen to ensure a single source of truth for infrastructure logic.
Need help? Check the Contributing Guide or ask in
GitHub Discussions!