-
Notifications
You must be signed in to change notification settings - Fork 1
Refactor cli, use zarr instead of N5 #73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
add single-scale ome-ngff (0.4) metadata generation when creating a …
🐛 write unit into a .zattrs metadata as a string, not as a list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the CLI architecture and migrates from N5 to Zarr format. The changes introduce dynamic CLI generation based on ModelConfig subclasses and consolidate model configuration classes into a dedicated module.
Changes:
- Migrated from N5 to Zarr format across server endpoints and neuroglancer integrations
- Refactored CLI to use dynamic command generation based on ModelConfig subclasses
- Moved model configuration classes from
utils/data.pytomodels/models_config.py - Introduced new utility modules for CLI generation, configuration serialization, and multi-cloud job extensions
- Updated documentation with comprehensive CLI reference guide
Reviewed changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| cellmap_flow/server.py | Changed from N5 to Zarr format for API endpoints and metadata |
| cellmap_flow/cli/cli.py | Refactored to use dynamic command generation from ModelConfig classes |
| cellmap_flow/cli/server_cli.py | Updated to dynamically create server commands |
| cellmap_flow/cli/yaml_cli.py | New YAML-based CLI with dynamic model discovery |
| cellmap_flow/models/models_config.py | New module consolidating all ModelConfig classes |
| cellmap_flow/utils/cli_utils.py | New utilities for dynamic CLI generation |
| cellmap_flow/utils/bsub_utils.py | Refactored with Job abstraction and cloud extensions |
| cellmap_flow/utils/ds.py | Added axes_names tracking and removed swap_axes logic |
| docs/source/cli_reference.rst | New comprehensive CLI documentation |
| neuroglancer_utils.py | Root-level file with Zarr migration |
Comments suppressed due to low confidence (2)
cellmap_flow/utils/ds.py:766
- get_ds_info returns tuple of size 5 and tuple of size 6.
get_ds_info returns tuple of size 5 and tuple of size 6.
get_ds_info returns tuple of size 5 and tuple of size 6.
get_ds_info returns tuple of size 5 and tuple of size 6.
get_ds_info returns tuple of size 2 and tuple of size 5.
get_ds_info returns tuple of size 2 and tuple of size 6.
get_ds_info returns tuple of size 2 and tuple of size 6.
get_ds_info returns tuple of size 2 and tuple of size 6.
get_ds_info returns tuple of size 2 and tuple of size 6.
def get_ds_info(path: str, mode: str = "r"):
example/server_check.py:8
- Import of 'ModelConfig' is not used.
Import of 'BioModelConfig' is not used.
Import of 'DaCapoModelConfig' is not used.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # if isinstance(v, np.ndarray): | ||
| # elms.append(f"{k}: type={type(v)} shape={v.shape}\n") | ||
| # elif inspect.ismodule(v): | ||
| # elms.append(f"{k}: <module '{v.__name__}'>\n") | ||
| # elif k=="checkpoint" or k=="model": | ||
| # elms.append(f"{k}\n") | ||
| # else: |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment appears to contain commented-out code.
|
|
||
| # Get type hints if available | ||
| try: | ||
| type_hints = get_type_hints(config_class.__init__) |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable type_hints is not used.
| # Get type hints if available | ||
| try: | ||
| type_hints = get_type_hints(config_class.__init__) | ||
| except: | ||
| type_hints = {} | ||
|
|
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable type_hints is not used.
| # Get type hints if available | |
| try: | |
| type_hints = get_type_hints(config_class.__init__) | |
| except: | |
| type_hints = {} |
| for job in g.jobs: | ||
| model = job.model_name | ||
| host = job.host | ||
| color = next(color_cycle) |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable color is not used.
|
|
||
| # Get type hints if available | ||
| try: | ||
| type_hints = get_type_hints(config_class.__init__) |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable type_hints is not used.
| from cellmap_flow.utils.cli_utils import ( | ||
| get_all_subclasses, | ||
| create_click_option_from_param, | ||
| process_constructor_args, | ||
| get_all_model_configs, | ||
| print_available_models, | ||
| ) |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'get_all_subclasses' is not used.
| # Get type hints if available | ||
| try: | ||
| type_hints = get_type_hints(config_class.__init__) | ||
| except: |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except block directly handles BaseException.
| except: | |
| except (TypeError, NameError, AttributeError): |
|
|
||
| try: | ||
| type_hints = get_type_hints(config_class.__init__) | ||
| except: |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except block directly handles BaseException.
| except: | |
| except Exception: |
| # Get type hints if available | ||
| try: | ||
| type_hints = get_type_hints(config_class.__init__) | ||
| except: |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except block directly handles BaseException.
| except: | |
| except Exception: |
| self.filetype, | ||
| ) = get_ds_info(dataset_path) |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left hand side of assignment contains 6 variables, but right hand side is a tuple of length 5.
| self.filetype, | |
| ) = get_ds_info(dataset_path) | |
| *maybe_filetype, | |
| ) = get_ds_info(dataset_path) | |
| self.filetype = maybe_filetype[0] if maybe_filetype else None |
Cli :