Perform a Python code review on the provided code. Accept input in any of the following forms:
Reviews Python code for Galaxy dependency injection best practices and suggests Lagom-based fixes.
/plugin marketplace add jmchilton/gx-arch-review/plugin install jmchilton-gx-arch-review@jmchilton/gx-arch-reviewPerform a Python code review on the provided code. Accept input in any of the following forms:
Review the code and ensure it follows best practices for dependency injection in Galaxy:
app accessapp property creation - Don't add new properties to the app object when DI can be usedGalaxy's app object was historically a "god object" - an object that knows/does too much. Every component, controller, and web transaction had a reference to app. This creates:
UniverseApplication, which creates that manager)app are needed, hard to mock)Galaxy uses Lagom for type-based DI:
Use StructuredApp interface instead of UniverseApplication to break circular dependencies:
# BAD - creates circular dependency
class MyManager:
def __init__(self, app: UniverseApplication):
...
# GOOD - uses interface, breaks cycle
class MyManager:
def __init__(self, app: StructuredApp):
...
# BETTER - inject only what you need
class MyManager:
def __init__(self, model: GalaxyModelMapping, security: IdEncodingHelper):
...
Anti-pattern - using app directly:
class DatasetCollectionManager:
def __init__(self, app):
self.model = app.model
self.security = app.security
self.hda_manager = hdas.HDAManager(app) # BAD: internal construction
self.history_manager = histories.HistoryManager(app)
Correct pattern - injecting dependencies:
class DatasetCollectionManager:
def __init__(
self,
model: GalaxyModelMapping,
security: IdEncodingHelper,
hda_manager: HDAManager,
history_manager: HistoryManager,
):
self.model = model
self.security = security
self.hda_manager = hda_manager
self.history_manager = history_manager
Benefits:
app objectAnti-pattern - FastAPI Depends():
def get_tags_manager() -> TagsManager:
return TagsManager()
@cbv(router)
class FastAPITags:
manager: TagsManager = Depends(get_tags_manager) # BAD: FastAPI style
Correct pattern - Galaxy's depends():
@cbv(router)
class FastAPITags:
manager: TagsManager = depends(TagsManager) # GOOD: Galaxy style
Anti-pattern:
class TagsController(BaseAPIController):
def __init__(self, app):
super().__init__(app)
self.manager = TagsManager() # BAD: manual construction
Correct pattern:
class TagsController(BaseGalaxyAPIController):
manager: TagsManager = depends(TagsManager) # GOOD: DI
Correct pattern - using @galaxy_task:
@celery_app.task(ignore_result=True)
@galaxy_task
def purge_hda(hda_manager: HDAManager, hda_id):
hda = hda_manager.by_id(hda_id)
hda_manager._purge(hda)
Dependencies are automatically injected based on type annotations.
Task with multiple dependencies:
@celery_app.task
@galaxy_task
def set_metadata(
hda_manager: HDAManager,
ldda_manager: LDDAManager,
dataset_id,
model_class='HistoryDatasetAssociation'
):
...
When reviewing, reference these Galaxy codebase locations for patterns:
lib/galaxy/di/ - DI container setuplib/galaxy/structured_app.py - StructuredApp interface definitionlib/galaxy/app.py - UniverseApplication implementationlib/galaxy/managers/ - Manager implementations using DIlib/galaxy/webapps/galaxy/api/ - FastAPI controllers using DIlib/galaxy/celery/tasks.py - Celery tasks using DIFor each file/change, check:
app access for dependencies - Does code access app.some_manager instead of injecting?StructuredApp instead of UniverseApplication where needed?depends() - Controllers use depends(Type) not FastAPI Depends()?@galaxy_task decorator with type-annotated parameters?For each issue found, report:
Summarize findings with counts by issue type and overall assessment of DI compliance.