Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the dashboard Flask app from a monolithic app.py into a blueprint-based route structure, introducing a shared state.py module for cross-route state (log streaming buffers, BBX generator state, and server URLs).
Changes:
- Split dashboard endpoints into dedicated Flask blueprints under
cellmap_flow/dashboard/routes/. - Added shared dashboard state in
cellmap_flow/dashboard/state.py(log streaming + BBX generator state + configurable paths). - Updated the main dashboard app to register blueprints and wire up log streaming support.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| cellmap_flow/dashboard/app.py | Creates the Flask app, attaches a log handler, and registers all dashboard blueprints. |
| cellmap_flow/dashboard/state.py | Introduces shared global state (log buffers/clients, BBX generator state, and helper for blockwise task dir). |
| cellmap_flow/dashboard/routes/index_page.py | Serves the main index page and injects global state into templates. |
| cellmap_flow/dashboard/routes/pipeline_builder_page.py | Serves the pipeline builder UI and hydrates UI state from g. |
| cellmap_flow/dashboard/routes/pipeline.py | Adds pipeline APIs (apply/validate/process, dataset path, blockwise config). |
| cellmap_flow/dashboard/routes/models.py | Adds model catalog and model-config related APIs. |
| cellmap_flow/dashboard/routes/blockwise.py | Adds blockwise pipeline validation, YAML generation, precheck, and submission endpoints. |
| cellmap_flow/dashboard/routes/bbx_generator.py | Adds BBX generator endpoints backed by a Neuroglancer viewer. |
| cellmap_flow/dashboard/routes/logging_routes.py | Adds SSE log streaming and serves a bbox JSON template. |
| cellmap_flow/dashboard/routes/init.py | Package marker for routes. |
Comments suppressed due to low confidence (1)
cellmap_flow/dashboard/app.py:48
- create_and_run_app() sets port=0, which lets the OS choose an ephemeral port; without explicitly logging/returning the chosen port this can make the dashboard hard to discover/connect to. Consider using get_free_port() (as elsewhere in the repo) or a configurable fixed port, and log the final bind address for users.
hostname = socket.gethostname()
port = 0
logger.warning(f"Host name: {hostname}")
app.run(host="0.0.0.0", port=port, debug=False, use_reloader=False)
| input_norms = get_input_normalizers() | ||
| output_postprocessors = get_postprocessors_list() | ||
| model_mergers = get_model_mergers_list() | ||
| model_catalog = g.model_catalog |
There was a problem hiding this comment.
index() assigns model_catalog = g.model_catalog and then mutates it by adding a "User" category. This modifies global state for all subsequent requests and endpoints that use g.model_catalog. Consider copying (e.g., dict(g.model_catalog)) before adding request-specific entries.
| model_catalog = g.model_catalog | |
| model_catalog = dict(g.model_catalog) |
| return { | ||
| "success": True, | ||
| "task_yaml": yaml_content, | ||
| "task_config": task_yaml, | ||
| "task_paths": yaml_paths, |
There was a problem hiding this comment.
When separate_bounding_boxes_zarrs is enabled, multiple bbox-specific YAML files are written, but the response still returns task_yaml/yaml_content from the base config. This means task_yaml/task_config won’t match the YAMLs referenced by task_paths. Consider returning per-bbox YAML/config (or a mapping of path->content) when generating multiple YAMLs.
| with viewer.txn() as s: | ||
| try: | ||
| annotations_layer = s.layers["annotations"] | ||
| if hasattr(annotations_layer, 'annotations'): | ||
| for ann in annotations_layer.annotations: |
There was a problem hiding this comment.
finalize_bbx_generation() reads s.layers["annotations"], but the layer created in start_bbx_generator() is named "bboxes". This prevents returning the final bounding boxes.
| model_entry = { | ||
| "name": model.get("name"), | ||
| **model.get("params", model.get("config", {})) |
There was a problem hiding this comment.
model_entry currently expands model.get("params", model.get("config", {})). If the frontend always includes a "params" key (even as an empty dict), the model's "config" will be ignored and the generated YAML will miss required model configuration. Prefer merging config+params (with params overriding) or falling back to config when params is empty.
| model_entry = { | |
| "name": model.get("name"), | |
| **model.get("params", model.get("config", {})) | |
| config = model.get("config") or {} | |
| if not isinstance(config, dict): | |
| config = {} | |
| params = model.get("params") | |
| if not params: | |
| combined_params = config | |
| else: | |
| if not isinstance(params, dict): | |
| params = {} | |
| combined_params = {**config, **params} | |
| model_entry = { | |
| "name": model.get("name"), | |
| **combined_params |
| if not pipeline.get("inputs") or len(pipeline["inputs"]) == 0: | ||
| return {"valid": False, "error": "No input nodes defined"} | ||
|
|
||
| if not pipeline.get("outputs") or len(pipeline["outputs"]) == 0: | ||
| return {"valid": False, "error": "No output nodes defined"} |
There was a problem hiding this comment.
This validation route returns plain dicts (defaulting to HTTP 200) even for validation failures. For an API endpoint, return jsonify(...) and use 4xx status codes for invalid pipelines so client-side error handling is reliable.
| with viewer.txn() as s: | ||
| try: | ||
| annotations_layer = s.layers["annotations"] | ||
| if hasattr(annotations_layer, 'annotations'): | ||
| for ann in annotations_layer.annotations: |
There was a problem hiding this comment.
The viewer creates an annotation layer named "bboxes", but status extraction reads from s.layers["annotations"]. This will raise KeyError and prevent returning any bounding boxes. Read from the same layer name that was created ("bboxes").
| # Build bsub command | ||
| cores_master = blockwise_config["params"]["nb_cores_master"] | ||
| charge_group = blockwise_config["params"]["charge_group"] | ||
| queue = blockwise_config["params"]["queue"] |
There was a problem hiding this comment.
Variable queue is not used.
| queue = blockwise_config["params"]["queue"] |
|
|
||
|
|
||
| @logging_bp.route("/api/templates/bbox-json") | ||
| def get_bbox_json_template(): |
There was a problem hiding this comment.
get_bbox_json_template returns tuple of size 2 and tuple of size 3.
| import yaml | ||
| from flask import Blueprint, request | ||
|
|
||
| from cellmap_flow.globals import g |
There was a problem hiding this comment.
Import of 'g' is not used.
| from cellmap_flow.globals import g |
| for client_queue in log_clients: | ||
| try: | ||
| client_queue.put_nowait(log_entry) | ||
| except queue.Full: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except queue.Full: | |
| except queue.Full: | |
| # Intentionally drop log entries when a client's queue is full. | |
| # Logging should never block or raise due to slow consumers. |
No description provided.