feat(scc): SCC analysis — closes #1#15
Open
galigaribaldi wants to merge 3 commits into
Open
Conversation
…d scc_analysis module: engine (Tarjan), diagnostics (fragmentation/isolation), orchestrator \n - Integrate SCC into graph post-build flow with SCC_CACHE in dependencies.py \n - Add GET /api/v1/network/topological/scc-analysis endpoint \n - Result: giant component 95.02% (10,561/11,115 nodes) → Phase 3 ready
Reviewer's GuideAdds a strongly connected components (SCC) analysis pipeline (Tarjan-based) that runs automatically after graph build, caches the results, and exposes them via a new API endpoint to determine Phase 3 readiness based on the size of the giant component. Sequence diagram for SCC analysis API endpoint and cachingsequenceDiagram
actor Client
participant API as FastAPI_app
participant Dep as get_or_build_graph
participant GraphBuilder as VFTGraphBuilder
participant Orchestrator as SCCOrchestrator
participant SCCCache as SCC_CACHE
participant ReportDep as get_scc_report
Client->>API: GET /api/v1/network/topological/scc-analysis
API->>Dep: get_or_build_graph(mode, tolerance_m)
alt Graph_not_in_GRAPH_CACHE
Dep->>GraphBuilder: build_graph(mode, tolerance_m)
GraphBuilder-->>Dep: G
Dep->>Orchestrator: SCCOrchestrator(G)
Orchestrator->>Orchestrator: analyze()
Orchestrator-->>Dep: scc_report, giant_component
Dep->>SCCCache: store {report, giant_component}
Dep-->>API: G
else Graph_in_GRAPH_CACHE
Dep-->>API: G
end
API->>ReportDep: get_scc_report(mode, tolerance_m)
ReportDep->>SCCCache: get(cache_key)
SCCCache-->>ReportDep: report | None
ReportDep-->>API: report
alt report is None
API-->>Client: HTTP 500 (SCC analysis no disponible)
else report available
API-->>Client: JSON {status, parametros, data}
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
src/api/dependencies.py,get_giant_componentcomputes thecache_keybut never returns anything fromSCC_CACHE, so the helper is currently unusable; you likely want to mirrorget_scc_reportand returnSCC_CACHE.get(cache_key, {}).get("giant_component").
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `src/api/dependencies.py`, `get_giant_component` computes the `cache_key` but never returns anything from `SCC_CACHE`, so the helper is currently unusable; you likely want to mirror `get_scc_report` and return `SCC_CACHE.get(cache_key, {}).get("giant_component")`.
## Individual Comments
### Comment 1
<location path="src/api/dependencies.py" line_range="41-44" />
<code_context>
\ No newline at end of file
+
+ # --- SCC: diagnóstico post-build ---
+ analyzer = SCCOrchestrator(G)
+ scc_report = analyzer.analyze()
+ giant = analyzer.get_giant_component()
+ SCC_CACHE[cache_key] = {"report": scc_report, "giant_component": giant}
+
+ return G
</code_context>
<issue_to_address>
**suggestion (performance):** SCC analysis runs synchronously in the event loop thread, which can block the API on large graphs.
Graph construction is already offloaded with `asyncio.to_thread`, but `SCCOrchestrator.analyze()` and `get_giant_component()` still run on the event loop. For large graphs, Tarjan + metric computation can block request handling. Please move the SCC work into the existing `to_thread` call, or wrap the SCC analysis in its own `asyncio.to_thread` to avoid blocking the event loop.
Suggested implementation:
```python
# --- SCC: diagnóstico post-build ---
def _run_scc_analysis(graph):
analyzer = SCCOrchestrator(graph)
report = analyzer.analyze()
giant = analyzer.get_giant_component()
return {"report": report, "giant_component": giant}
scc_result = await asyncio.to_thread(_run_scc_analysis, G)
SCC_CACHE[cache_key] = scc_result
return G
```
- This change assumes `asyncio` is already imported in `src/api/dependencies.py`. If not, add `import asyncio` at the top of the file.
- If there is a shared pattern or helper for CPU-bound work (e.g. a central `run_in_threadpool` wrapper), you may want to replace `asyncio.to_thread` with that to keep consistency with the rest of the codebase.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+41
to
+44
| analyzer = SCCOrchestrator(G) | ||
| scc_report = analyzer.analyze() | ||
| giant = analyzer.get_giant_component() | ||
| SCC_CACHE[cache_key] = {"report": scc_report, "giant_component": giant} |
There was a problem hiding this comment.
suggestion (performance): SCC analysis runs synchronously in the event loop thread, which can block the API on large graphs.
Graph construction is already offloaded with asyncio.to_thread, but SCCOrchestrator.analyze() and get_giant_component() still run on the event loop. For large graphs, Tarjan + metric computation can block request handling. Please move the SCC work into the existing to_thread call, or wrap the SCC analysis in its own asyncio.to_thread to avoid blocking the event loop.
Suggested implementation:
# --- SCC: diagnóstico post-build ---
def _run_scc_analysis(graph):
analyzer = SCCOrchestrator(graph)
report = analyzer.analyze()
giant = analyzer.get_giant_component()
return {"report": report, "giant_component": giant}
scc_result = await asyncio.to_thread(_run_scc_analysis, G)
SCC_CACHE[cache_key] = scc_result
return G- This change assumes
asynciois already imported insrc/api/dependencies.py. If not, addimport asyncioat the top of the file. - If there is a shared pattern or helper for CPU-bound work (e.g. a central
run_in_threadpoolwrapper), you may want to replaceasyncio.to_threadwith that to keep consistency with the rest of the codebase.
- Restructure topologicalIndicators/ into topological/, spatial/, demand/ \n- Add @phase: header to all indicator files for roadmap traceability \n- Update 13 imports across API layer (main.py, dependencies.py, geo_layers.py) \n- Update 4 imports in notebooks (02, 03, 04) \n- Rewrite Description.md with new architecture and domain taxonomy \n- Add demand/ placeholder for future Apimetro Plutarco indicators
- Restructure topologicalIndicators/ into topological/, spatial/, demand/ \n- Add @phase: header to all indicator files for roadmap traceability \n- Update 13 imports across API layer (main.py, dependencies.py, geo_layers.py) \n- Update 4 imports in notebooks (02, 03, 04) \n- Rewrite Description.md with new architecture and domain taxonomy \n- Add demand/ placeholder for future Apimetro Plutarco indicators
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
scc_analysis/con motor Tarjan, diagnósticos por sistema y orquestadorSCC_CACHE)GET /api/v1/network/topological/scc-analysisTest plan
make run+GET /api/v1/network/build-auto→ grafo en cachéGET /api/v1/network/topological/scc-analysis→phase_3_ready: trueCloses Verificar SCC del grafo antes de implementar T y B #1
Summary by Sourcery
Add SCC-based topological connectivity analysis for the network graph and expose it via a new API endpoint, caching results alongside the built graph.
New Features:
Enhancements: