refactor: remove unused public API - #633
Conversation
Nine public items across the three crates had no consumer in this repo, none in dynamo (checked against its origin/main), and the crates carry two, three and zero reverse dependencies on crates.io. modelexpress_common: - trait ConfigLoader and its load_layered method, which had no impl and no caller. The real path is the free fn load_layered_config, which stays - CacheConfig::from_server, a permanent Err stub - the POD_NAME and POD_UID name constants. POD_NAMESPACE stays, it is read in backend_config.rs. Both removed names remain live in the Python client, which defines and consumes them, so no doc change is warranted - SealedArtifactManifest::to_header_response and to_chunks_response. These build WorkerService responses; no Rust implementation of that service exists and the byte-transfer follow-up landed in Python - the Network variant of the shared Error enum. The seven sibling variants stay - ClientConfig::with_cache_path. Its with_endpoint, with_timeout and for_testing siblings are live and stay modelexpress_client: - five Client cache-management methods: get_cache_config, set_cache_config, list_cached_models, clear_cached_model, clear_all_cached_models. The CLI reaches CacheConfig directly rather than going through any of them modelexpress_server: - CacheEvictionService::manual_evict - the three-argument set_status wrapper that only delegated to set_status_and_notify with no message. The RegistryBackend trait method of the same name takes four arguments and is untouched Three unit tests go with the items they covered, along with a test helper and a page-size constant used only by to_chunks_response. EvictionReason::Manual is left in place. It becomes unconstructable here, but CountLimit and DiskSpace already were, so removing only Manual would be arbitrary and removing all three is a question about whether those eviction policies are still planned. Verified with cargo clippy --workspace --all-targets --all-features -D warnings, which is the load-bearing check since it compiles every target, and with the lib suites at 35, 176 and 248 passing. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (8)
WalkthroughThe change removes unused public APIs for cache management, artifact-manifest pagination, layered configuration, pod environment variables, and network errors. Related imports, tests, fixtures, and implementation paths are also removed. ChangesPublic API cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to This PR removes unused public API while preserving the documented live usages, and the reported checks pass; no actionable merge-blocking risk remains beyond normal review. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
| /// Kubernetes pod name injected via the downward API (used by clients). | ||
| pub const POD_NAME: &str = "POD_NAME"; | ||
| /// Kubernetes pod UID injected via the downward API (used by clients). | ||
| pub const POD_UID: &str = "POD_UID"; |
There was a problem hiding this comment.
These are in use by ownerReference
There was a problem hiding this comment.
fair, and the doc comment i deleted said "used by clients" which should have stopped me.
but as far as i can tell the ownerReference path runs entirely through python: the downward api sets these on the worker, envs.py declares them, client.py sends pod_name and pod_uid on the request, and pod_owner_references takes them off the request rather than off the server's own environment. i can't find a rust reader for either constant, so the removal should be behaviourally inert.
which leaves the question of what envs.rs is for. POD_NAMESPACE is read in rust at backend_config.rs, so those three sit together today with two of them declaration-only. is envs.rs meant to be the canonical registry for python-only var names as well, or should those live only in envs.py? if it's the registry, i'll put both back and leave the trio alone. if it's rust readers only, POD_NAMESPACE is the one that's correctly there and the other two were always redundant with envs.py.
holding the removal as-is until you say which.
Nine public items across the three crates have no consumer in this repository and none in dynamo, checked against its origin/main because the local checkout is far behind. The crates carry two, three and zero reverse dependencies on crates.io.
In modelexpress_common: the ConfigLoader trait and its load_layered method, which had no impl and no caller, the real path being the free function load_layered_config which stays; CacheConfig::from_server, a permanent Err stub; the POD_NAME and POD_UID name constants, with POD_NAMESPACE staying because backend_config.rs reads it; SealedArtifactManifest::to_header_response and to_chunks_response, which build WorkerService responses for a service with no Rust implementation; the Network variant of the shared Error enum, whose other seven variants stay; and ClientConfig::with_cache_path, whose with_endpoint, with_timeout and for_testing counterparts are live and stay.
In modelexpress_client: five Client cache-management methods, get_cache_config, set_cache_config, list_cached_models, clear_cached_model and clear_all_cached_models. The CLI reaches CacheConfig directly and never calls any of them.
In modelexpress_server: CacheEvictionService::manual_evict, and the three-argument set_status wrapper that only delegated to set_status_and_notify with no message. The RegistryBackend trait method of the same name takes four arguments and is untouched.
POD_NAME and POD_UID remain live in the Python client, which defines and consumes them, so the Downward API documentation is still accurate and unchanged. EvictionReason::Manual is left in place: it becomes unconstructable here, but CountLimit and DiskSpace already were, so removing only Manual would be arbitrary and removing all three is a question about whether those eviction policies are still planned.
Three unit tests go with the items they covered, along with a test helper and a page-size constant used only by to_chunks_response. Verified with cargo clippy --workspace --all-targets --all-features -D warnings, which is the load-bearing check because it compiles every target, and with the lib suites at 35, 176 and 248 passing.
Summary by CodeRabbit
Refactor
Breaking Changes