Fix hardcoded model lookup and fragile parsing in split_inference (#47)#65
Open
ff225 wants to merge 1 commit into
Open
Fix hardcoded model lookup and fragile parsing in split_inference (#47)#65ff225 wants to merge 1 commit into
ff225 wants to merge 1 commit into
Conversation
The /api/split_inference endpoint (issue #47) duplicated a hardcoded {size: folder_name} mapping instead of reusing settings.yaml's "model" section, and derived the device size with a fragile device_id.split("_")[1].split("x")[0] that raised an unhandled IndexError on malformed input. - Resolve model_dir via settings.yaml's "model" section (same source RequestHandler/ModelManager use), removing the duplicated dict. - Parse device size with a validating regex, returning a clean 400 on malformed device_id instead of an unhandled exception. - Leave the skip_connections dict in place with a comment: it encodes the FOMO submodel's fixed network topology, not deployment config, so migrating it into settings.yaml wouldn't add value. Full integration with RequestHandler/OffloadingAlgo/ModelManager's request flow was considered and scoped out: this endpoint's synchronous request/response shape (decode tensor, run remaining layers, return prediction) doesn't match the main pipeline's async device-registration and telemetry flow, so folding it in would be a disproportionate rewrite for what's still a validation/prototype endpoint. It keeps tests/test_accuracy/test_ImageWithBox.py's split-vs-local accuracy comparison working across all 5 FOMO sizes. Adds tests/integration/test_split_inference_validation.py, a fast, artifact-free test covering the new validation paths (malformed device_id, unknown model size), so this endpoint has coverage in normal CI beyond the artifact-gated accuracy suite.
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
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.
Decision: fix-in-place, not full pipeline integration
Closes #47.
I read through
src/server/communication/http_server.py'ssplit_inferencehandler plusModelManager,RequestHandler, andOffloadingAlgoto judge feasibility. Verdict: fix the cited code-quality issues in place; do not fold this endpoint into the mainRequestHandler/OffloadingAlgo/ModelManagerpipeline in this change.Why fix-in-place instead of full integration:
RequestHandler.handle_device_inference_result,OffloadingAlgo, CSV/Firestore telemetry).split_inferenceis a synchronous "decode tensor in, run remaining TFLite submodels, return prediction" request with no device registration or telemetry semantics at all — reshaping it to fit the main pipeline's data model would be a disproportionate rewrite for what's still a validation/prototype endpoint, with real risk of destabilizing the main pipeline for no behavioral benefit.tests/test_accuracy/test_ImageWithBox.pyuses it as ground truth to validate that server-side split inference matches local end-to-end inference, across all 5 FOMO model sizes (48/96/192/224/300). That test is gated behindSCIOT_RUN_ACCURACY_TESTS=1plusmodel_artifact/external_servermarkers (so it doesn't run in normal CI), but the model artifacts it needs are checked into the repo and it is a real, working accuracy harness. Removing the endpoint would silently delete that coverage, which the issue does not ask for and which I don't think is the right call given the artifacts already exist and the test already works.What changed, issue point by issue point
nomi_cartelle = {"48": "FOMO_48_CUT", ...}): removed. The handler now resolvesmodel_dirfromsettings.yaml's existingmodelsection (fomo_48x48.model_dir, etc.) via a newHttpServer._resolve_model_dir(size)helper — the same config sectionRequestHandler/ModelManageralready read (request_handler.py:655). Single source of truth, no more drift risk between the two hardcoded dicts.device_id.split("_")[1].split("x")[0]): replaced with a validating regex (HttpServer._parse_device_size) that returnsNoneon a malformeddevice_idinstead of raising an unhandledIndexError; the handler now returns a clean400with a descriptive message in that case.{25: [16, 24], 43: [34, 42], 52: [43, 51]}): left in place, but now with a comment explaining why: these indices describe the FOMO submodel's fixed network topology as exported, not a deployment/runtime setting — there is nothing insettings.yamlorModelManagerthat models network topology today, and inventing that config surface for one endpoint would be scope creep beyond this issue.RequestHandler/OffloadingAlgo/Edge.run_inference/ModelManager: documented above as an explicit, considered decision, not an oversight.Test coverage
Added
tests/integration/test_split_inference_validation.py: fast, artifact-free tests (using the existingtiny_http_serverfixture, no TFLite model files or external server needed) covering:device_id(no size, trailing underscore, missing second dimension, empty) ->400with a clear message400with the existing "non trovato" messageHttpServer._parse_device_sizehelper directlyThese run in normal CI (
tests/integration/, noexternal_server/model_artifactmarkers), unlike the pre-existing artifact-gated accuracy test, so this endpoint now has always-on regression coverage for its request-validation paths.Testing performed
uv run pytest tests/integration/test_split_inference_validation.py -q— 7 passeduv run pytest tests/integration/test_http_end_to_end.py tests/integration/test_http_protocol_validation.py tests/integration/test_split_inference_validation.py -q— 15 passedtests/unit,tests/integration/test_client_resilience.py,test_client_lifecycle.py,test_variance_and_local_inference.py,tests/test_mqtt_client,tests/test_offloading_algo) plus the above HTTP suites — 199 passed, 1 skippedtests/test_accuracy/test_ImageWithBox.py --collect-only— still collects 5 tests unaffected (device_id formatdevice_{size}x{size}still matches the new parser)