refactor(project-service): split the 15k-line ProjectService god-class into project/core/ - #32
Open
martin-s-a wants to merge 14 commits into
Open
refactor(project-service): split the 15k-line ProjectService god-class into project/core/#32martin-s-a wants to merge 14 commits into
martin-s-a wants to merge 14 commits into
Conversation
…ewers into project/core/ Phase 1 of splitting the 15k-line ProjectService god-class into focused, single-responsibility modules under app/backend/api/services/project/core/, per the AST call-graph analysis done earlier this session. ProjectService keeps every public/private method name and signature unchanged - bodies become thin delegations - so the router and the two external "private method" consumers (protocol_service.py, protocol_steps_sync.py) are untouched. - core/tags.py, core/fsc_preview.py, core/external_viewers.py (new) - Fixed 7 pre-existing test regressions from the earlier "core" helper extraction (generated_set_helpers/protocol_resolution): tests that monkeypatched ProtocolIdentityResolver/RuntimeProtocolOutputPersistenceService or _resolvePostgresqlProtocolDbId needed their patch target updated to where that logic actually moved. - Added test_project_service_external_viewers.py (20 tests) - that ~570 line section had zero prior coverage, discovered while extracting it. - Removed now-dead Config/DESKTOP_TKINTER imports from project_service.py. 270/270 (targeted files) and 1043/1043 (full unit suite) passing, matching the pre-refactor baseline exactly plus the new coverage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ct-service # Conflicts: # app/backend/api/services/project_service.py
…preview helpers into project/core/
Phase 2 of the ProjectService split. Per an AST call-graph analysis
(rebuilt fresh this pass, since the two previous devel merges touched
the file), these three preview domains turned out to have zero real
call-graph overlap with each other - each only shares already-extracted
generic infra (_resolvePostgresqlReaderProtocolId, _getScipionProtocolForRuntime,
_getPostgresqlRuntimeOutputInfo) and, for Coordinates3D, two 2D-slice
pixel helpers also used by Volumes/metadata preview (Phase 3 territory).
- core/ctftomo_preview.py, core/coords3d_preview.py, core/tiltseries_preview.py:
the pure summary/measurement-row builders, static tomogram iterator,
frame-path parser, tilt-image cloner, and PostgreSQL reader construction
for each domain.
- core/slice_rendering.py: normalize2dSlice/coords3dPilTo2dTile pulled out
now (fully self-contained, zero coupling) rather than deferred to Phase 3,
since Coordinates3D already needed them and moving them once is cheaper
than twice.
- The module-level tilt-series preview cache (_tiltSeriesPreviewCache/
_tiltSeriesPreviewCacheLock/_TILT_SERIES_PREVIEW_CACHE_LIMIT) moved into
core/tiltseries_preview.py wholesale - nothing outside project_service.py
referenced those names directly (confirmed via grep before moving).
- Left _resolveOutputFor{CtftomoSeries,Coordinates3d,TiltSeries} and the
~10 large orchestration Service methods (listOutput*/get*/render*/create*,
174-300 lines each) on ProjectService itself for now: tests monkeypatch
several of them directly at the instance level (e.g.
test_project_service_ctftomo.py patches _resolveOutputForCtftomoSeries),
and they still call other not-yet-extracted shared cross-phase helpers
(_getPostgresqlRuntimeOutputInfo, _renderTomogramSliceFromPath) that
Volumes/metadata (Phase 3) also depend on - extracting them now would mean
redoing the split once that shared dispatch layer exists.
1077/1077 unit tests passing, matching baseline exactly (no behavior change).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…slice-rendering into project/core/ Phase 3 of the ProjectService split (Volumes + metadata-table preview, the two biggest remaining preview blocks per the original plan). - core/volume_downsampling.py: the pure numpy/FFT volume downsampling pipeline (binning/linear/Fourier-crop + the stride-based variant used for surface meshes) - no Scipion/PostgreSQL state anywhere in it. - core/volume_preview.py: volume path resolution, numpy volume reading, the module-level volume-slice response cache (moved wholesale, nothing outside project_service.py referenced its names), and PostgreSQL volume reader construction. - core/metadata_preview.py: generic metadata-table introspection/cell rendering/selection-id helpers and PostgreSQL DAO construction. - core/slice_rendering.py (extended): _renderTomogramSliceFromPath moved in whole - it turned out to have zero self-dispatch dependencies once read closely (only called the already-extracted coords3dPilTo2dTile/ normalize2dSlice), so this closes out the Phase-2 "shared dispatch layer" deferral for the 2D-slice-rendering path specifically. - Two more monkeypatch-relocation fixes, same root cause as earlier phases: _getVolumePathFromOutput's isinstance(output, SetOfVolumes) check now takes the class as an explicit parameter (the ProjectService method passes its own module-level SetOfVolumes binding, the one tests patch) instead of the core module importing its own; same treatment for _buildMetadataSelectionArgument's OBJECT_TABLE comparison. - Deleted _centerCrop3d/_resizeVolumeFourier/_strideDownsampleVolume as ProjectService methods - confirmed zero callers anywhere in the repo (including tests) now that _resizeVolumeFourier's only caller uses the pure module function directly; kept _binVolume/_downsampleVolumePreview since a test calls them directly on the service instance. - Dropped 5 dead imports (NumpyDao, ScipionImageReader, ScipionSetsDAO, StarFile, SetOfTiltSeries) left over from the Phase-0/1 extractions that had already moved their only usages into core/ modules. Left the ~10 large orchestration methods (listOutputVolumesService, getVolumeData3dService, getVolumeSurfaceMesh, renderVolumeSliceService, runMetadataTableActionService, renderMetadataImageCellService, etc., 75-353 lines each) on ProjectService: several depend on self.currentProject directly or on _outputPreviewRuntime/_resolvePostgresqlOutputForPreview, which remain unextracted generic dispatch infra (still self.currentProject- coupled) - genuine Phase 5+ territory now that this pass covers every preview domain in the original Phase 1-3 plan. 1077/1077 unit tests passing, matching baseline exactly. project_service.py now 12670 lines, down from ~15176 at the start of this refactor. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ode, extract integrated-analyze-context runtime fallback
Phase 4 of the ProjectService split (filesystem navigation + PostgreSQL
tomo-relations, per the plan).
Filesystem navigation (getProtocolPath/listProtocolDir/previewProtocol*/
writeRemoteFileService, ~250 lines) turned out to already be thin
orchestration delegating its real complexity to the existing FileHandlers
helper class - confirmed via the AST call-graph analysis, this is exactly
the "small enough to stay one file" case the plan itself anticipated.
No extraction made there; a new core module would only relocate a dozen
lines of pure code while leaving the actual self-dispatch chains in place.
The PostgreSQL tomo-relations side had a bigger finding: the AST fan-in
data showed _getPostgresqlStoredSetForIntegratedContext,
_getPostgresqlIntegratedKind, _getPostgresqlStoredSetProperty,
_buildPostgresqlIntegratedLink, _buildPostgresqlIntegratedSummary,
_firstPostgresqlValueByName, _addPostgresqlIntegratedRelation, and its
4 per-type callers (_addPostgresql{TiltSeries,Ctftomo,Tomogram,
Coordinates3d}Relations) - 11 methods, ~240 lines - had zero callers
anywhere in the repo, not even tests. Confirmed via repo-wide grep before
deleting: this whole manual dict-based relation-builder was superseded by
PostgresqlIntegratedContextReader, which getIntegratedAnalyzeContextService
already calls directly for the PostgreSQL-native path. Deleted outright
rather than "extracted", since extracting dead code just relocates the
deadness.
getIntegratedAnalyzeContextService's ~400-line runtime-fallback body (used
when no PostgreSQL mapper is available) turned out to be a single pure
computation over a live Scipion protocol graph, with only two self.
touches (self.currentProject, self._safeScipionValue) - both now passed/
imported explicitly. Moved whole into
core/integrated_analyze_context.py::buildIntegratedAnalyzeContextFromRuntime.
Kept as one file despite landing at 490 lines: it's a single cohesive
function built from tightly interdependent local closures (buildLink/
buildSummary/findInputRefForObject/etc. all share className/safeCall/
getTsIds) that don't decompose cleanly into separate call-graph-verified
pieces the way the Phase 1-3 preview domains did.
1077/1077 unit tests passing, matching baseline exactly. project_service.py
now 11970 lines, down from ~15176 at the start of this refactor.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…to project/core/
Phase 5 of the ProjectService split (protocol graph + protocol params/
launch/steps, per the plan).
- core/protocol_graph_builder.py: buildProtocolsGraph (the ~430-line
method the plan called out as "a real unit") plus its pure numeric/
string helpers (toPersistedOutputInt/Float, elapsed-seconds formatting,
thumbnail URL builders). Confirmed via test-file grep that nothing
monkeypatches its self-dispatched pieces, so it's a fully pure function
taking currentProject explicitly.
- core/protocol_graph_load.py: the PostgreSQL-only graph-data loader
(tags/dependencies/rows/persisted-outputs/step-summaries). One
monkeypatch-relocation fix here: a test patches
service._loadPersistedOutputsByProtocolId directly, so that step takes
an optional callback (defaulting to the real runtime service) instead
of the pure function hardcoding it - same fix pattern as earlier phases.
- Split into two files rather than one ~630-line module, matching the
plan's own ProtocolGraphBuilderService/ProtocolGraphLoadService split.
Two sub-scopes from the plan turned out not to need action, confirmed by
close reading rather than assumed:
- "WorkflowTemplateService (listing/applying workflow templates)":
listProjectWorkflows/applyWorkflowToProject turned out to be entry
points into the same ~1900-line workflow import/export machinery
(_prepareWorkflowFileForImport, _workflowProtocolMapToProtocols,
_buildImportedWorkflowPointerParamsByProtocolId, etc.) that the plan
already assigns to Phase 6 - not a separable concern. Deferred there
rather than forcing a partial extraction now.
- "Protocol params/launch/steps (~700 lines)": every method in this area
(getNewProtocolParams, getProtocolParams, applyParamsToProtocol,
saveProtocol, launchProtocol, listProtocolStepsService, ...) was
already a thin callback-wiring delegator to already-extracted
app/backend/runtime/* services. Nothing left to extract - the plan's
own text anticipated this ("may be small enough that further splitting
isn't worth it").
1077/1077 unit tests passing, matching baseline exactly. project_service.py
now 11433 lines, down from ~15176 at the start of this refactor.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ow export/import into project/core/ Phase 6 of the ProjectService split (protocol delete/restart/workflow import-export, per the plan - now the biggest remaining piece since devel's own cleanup had already stripped the legacy protocol-lifecycle branches this phase originally expected to deal with). - core/protocol_delete_cleanup.py: the protocol-working-directory delete guard (symlink/path-escape checks) and the PostgreSQL-runtime-artifact + filesystem cleanup that runs after a protocol delete. - core/workflow_export.py: building the ScipionWeb export header/JSON content and per-protocol plugin metadata. - core/workflow_import.py: parsing/validating a workflow file for import - embedded-JSON extraction, required-plugin availability checking, reading a ScipionWeb export payload. - core/workflow_pointer_remap.py: split out of workflow_import.py once it passed ~600 lines - protocol-id bookkeeping across an import and remapping pointer/relation params from the workflow's original ids onto the newly-imported protocols' real ids. Matches the plan's own WorkflowExportService/WorkflowImportService/WorkflowNormalizeHelpers split. Restart/continue/stop (_launchPostgresqlRestartSubworkflow, restartProtocolAll, continueProtocolAll, stopProtocol, ...) turned out to already be thin delegation to already-extracted app/backend/runtime/* services - nothing to extract, confirming the devel-cleanup effect noted after the earlier merge. Systematically checked every candidate method against all test files for direct monkeypatch.setattr(service, "...") before extracting (not just apply_workflow's own tests - protocols/io_thumbnails tests patch a few of these too). ~10 methods stayed as thin self-dispatching delegators because tests replace them wholesale (_prepareWorkflowFileForImport, _buildImportedWorkflowPointerParamsByProtocolId, _normalizeWorkflowImportErrors, _workflowProtocolMapToProtocols, _getProtocolObjIdForExport, _resolveRuntimeProtocolsForExport, _restorePostgresqlRuntimePointersForProtocols, _syncImportedPostgresqlRuntimeProtocols, listProjectWorkflows) - their bodies still moved to core/, only the outer method stayed on ProjectService as the patchable seam. Left applyWorkflowToProject/importWorkflowProtocolsService/ exportWorkflowProtocolsService/exportProtocolsService/ _prepareRuntimeProtocolsForExport/_getPostgresqlRuntimeSubworkflow on ProjectService: heavy self.currentProject/mapper coupling and orchestration across many of the now-extracted pure pieces, not further reducible without duplicating Phase 7's project-loading scope. Two dead imports removed (RelationParam, uuid4) whose only usages moved into core/ modules with their own local imports. 1077/1077 unit tests passing, matching baseline exactly. project_service.py now 10532 lines, down from ~15176 at the start of this refactor. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…into project/core/ Phase 7 of the ProjectService split (project CRUD/import/export/sharing, per the plan). - core/project_sharing.py: share/list-shares/revoke - fully pure, zero self-state, matches the plan's small standalone ProjectSharingService. - core/project_display.py: building the JSON project summary row (display name, disk usage, protocol count, thumbnail urls). - core/project_path.py: sanitizing project names and resolving/guarding project filesystem paths against the managed projects root - manager (a pyworkflow Manager) is always taken explicitly rather than cached. - core/project_import_validation.py: validating a folder is a real, loadable Scipion project for the "import existing project" flow. - core/project_crud.py: create/update/delete, composed from the above four modules. One non-test-file fix this phase: a static-analysis safety guard (test_runtime_project_loading_guard.py) scans the whole services/runtime tree for legacy project.load(dbPath=...) calls via an exact (file, function name) allowlist - the same "test tracks a specific location" pattern as the monkeypatch-relocation fixes from earlier phases, just via a different mechanism. Updated its allowlist entry to point at the new project_import_validation.py location, and updated its own self-test (which had synthetic source hard-coded to the old location/name) to match. Left importProject/_migrateImportedProjectToPostgresql/ _syncLegacyProjectGraphToPostgresql/loadProjectFromPostgresql/ getProjectEffectiveSettings/getProjectDbRow on ProjectService: genuine composition-root orchestration with heavy self.currentProject/mapper coupling, real side effects (sqlite3, mapper transactions, filesystem), and several directly monkeypatched in tests - the same category consistently left in place since Phase 2 (applyWorkflowToProject, exportWorkflowProtocolsService, getVolumeData3dService, etc.). _resolvePostgresqlProjectPathForFilesystem/_buildMissingOutputSyncItems were already thin orchestration around already-extracted runtime/* services - nothing to extract there either. Removed 1 dead import (shutil) whose only usage moved into project_path.py. 1077/1077 unit tests passing, matching baseline exactly. project_service.py now 9974 lines, down from ~15176 at the start of this refactor - under 10k for the first time. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ates Phase 8 of the ProjectService split - final assembly verification and documentation, per the plan. Verification performed (no code changes needed, everything already held): - Full unit suite: 1077/1077 passing. - Module imports cleanly standalone and with the real routers/services that depend on it (project_router.py, protocol_router.py, protocol_service.py, protocol_steps_sync.py, coords2d_service.py all import and resolve their ProjectService method calls correctly). - The two external "private method" consumers still resolve: protocol_service.py's ProjectService()._loadPostgresqlRuntimeProject and protocol_steps_sync.py's service._shouldRegisterProtocolOutputs/ registerOutput are all still defined (the latter two as thin delegators to core/output_registration.py, established back in an earlier phase). - _thumbnailProjectLock (imported directly by project_router.py) is untouched - still the right home for it, no relocation needed. - Spot-checked every project/core/ module's line count: ~30 files, ~5900 lines total, all under ~610 lines. Updated .ai/tech-debt.md: replaced the original 15176-line "single largest god file" finding with the current state (9974 lines, down ~34%, split into ~30 focused core/ modules) - still the largest file in the ecosystem, but the character of what remains changed from undifferentiated bulk to composition-root orchestration. Updated .ai/roadmap.md: marked the split done, and captured the two follow-ups deliberately deferred rather than bundled into this pass - migrating project_router.py's ~90 endpoints and the two external "private method" call sites to depend on project/core/ or app/backend/runtime/* directly, and a possible future look at external_viewers.py/protocol_graph_builder.py (~500-610 lines, the two modules over the original target) if either grows further. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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
app/backend/api/services/project_service.pywas a single 15176-lineProjectServiceclass - by far the largest file in the whole ecosystem. This splits it across 8 phases into ~30 focused, independently-testable modules underapp/backend/api/services/project/core/, while keepingProjectService's public/private method names and signatures unchanged so routers and the external "private method" consumers (protocol_service.py,protocol_steps_sync.py,coords2d_service.py) don't need to change.PostgresqlIntegratedContextReader) along the way.ai/tech-debt.md/.ai/roadmap.mdupdatesproject_service.py: 15176 → 9974 lines (-34%). What remains is genuine composition-root orchestration (methods wiring together the newcore/modules, mutating the few real instance-state fields, or directly monkeypatched by name in tests) rather than undifferentiated bulk.Two follow-ups deliberately deferred, documented in
.ai/roadmap.md:project_router.py's ~90 endpoints and the two external "private method" call sites to depend onproject/core//app/backend/runtime/*directly instead of going throughProjectService.external_viewers.py(~612 lines) andprotocol_graph_builder.py(~501 lines), the twocore/modules that landed over the original ~200-500 line target.Test plan
project_service.pyand all newproject/core/modules import cleanly standaloneProjectServiceimport cleanly and resolve their method calls:project_router.py,protocol_router.py,protocol_service.py,protocol_steps_sync.py,coords2d_service.py_loadPostgresqlRuntimeProject,_shouldRegisterProtocolOutputs,registerOutput)project/core/module's line count (~30 files, ~5900 lines total, all under ~610 lines)🤖 Generated with Claude Code