-
Notifications
You must be signed in to change notification settings - Fork 4
Feat/custom pipeline nicegui 3 3 1 #268
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?
Feat/custom pipeline nicegui 3 3 1 #268
Conversation
…ncel_and_download_and_delete against production
|
Claude finished @helmut-hoffer-von-ankershoffen's task —— View job PR Review: Feat/custom pipeline nicegui 3_3_1I've completed a comprehensive review of PR #268 following the Aignostics Python SDK standards for medical device software. ✅ Summary: APPROVED with SuggestionsThis PR successfully adds configurable pipeline orchestration (GPU/CPU resource control) to the SDK metadata schema, with appropriate CLI and GUI interfaces. All critical checks pass. Key Achievements:
📊 Changes OverviewFiles Changed: 9 files (+499/-70 lines)
🔍 Detailed Review1. ✅ Test Markers (CRITICAL CHECK - PASSED)Status: ✅ All tests properly marked Verified no unmarked tests exist: uv run pytest -m "not unit and not integration and not e2e" --collect-only
# Result: 0 tests selected ✅All 7 new tests in
2. ✅ Code Quality Checks (ALL PASSED)Linting: ✅ PASSED Tests: ✅ 7/7 new tests PASSED in 3.07s 3.
|
| user_info | ||
| and user_info.organization | ||
| and user_info.organization.name | ||
| and user_info.organization.name.lower() in {"aignostics", "pre-alpha-org", "lmu", "charite"} |
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.
Security & Scalability Consideration:
Hardcoding organization names in the frontend for feature gating has maintainability implications:
user_info.organization.name.lower() in {"aignostics", "pre-alpha-org", "lmu", "charite"}Suggestions:
- Backend Authorization: Move this logic to the API backend and check via a user permission/feature flag
- Configuration-based: Store allowed organizations in
_settings.pyor environment variables - Feature Flag Service: Use a feature flag system for gradual rollout
Current Risk: Low (alpha feature with known orgs)
Future Risk: Medium (requires code deployment for each new organization)
Why it matters: As this feature rolls out to more organizations, changing code for each org request is not sustainable for a medical device SDK.
| validate_only: Annotated[ | ||
| bool, typer.Option(help="If True, cancel the run post validation, before analysis.") | ||
| ] = False, | ||
| gpu_type: Annotated[ |
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.
Enhancement Suggestion:
Consider using enum types directly for validation instead of strings:
from aignostics.platform._sdk_metadata import GPUType, ProvisioningMode
gpu_type: Annotated[
GPUType, # Use enum directly
typer.Option(help="GPU type to use for processing."),
] = GPUType.A100Benefits:
- ✅ Type safety at runtime
- ✅ IDE autocomplete support
- ✅ Automatic validation (rejects invalid values like "A200")
- ✅ Consistent with internal models
Current approach is acceptable - this is an enhancement, not a bug.
| ITEM_SDK_METADATA_SCHEMA_VERSION = "0.0.3" | ||
|
|
||
| # Pipeline orchestration defaults | ||
| DEFAULT_GPU_TYPE = "A100" |
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.
Excellent Design Choice ✅
Defining pipeline defaults as module-level constants:
- ✅ Makes defaults explicit and discoverable
- ✅ Easier to update across all usages
- ✅ Testable (constants can be imported in tests)
- ✅ Self-documenting for SDK users
This follows best practices for configuration management in enterprise software.
| validate_item_sdk_metadata(metadata) | ||
|
|
||
|
|
||
| class TestPipelineConfiguration: |
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.
Excellent Test Coverage ✅
This test class demonstrates best practices for medical device software testing:
- ✅ Comprehensive coverage: Defaults, custom values, enums, validation, edge cases
- ✅ Clear test names: Each test describes exactly what it validates
- ✅ Proper markers: All tests correctly marked as
@pytest.mark.unit - ✅ Edge case testing: Invalid values (
max_gpus_per_slide=0,-1) properly tested - ✅ Integration testing: Tests both with and without pipeline config (optional field)
- ✅ Validation testing: Ensures Pydantic
PositiveIntconstraint works
Test execution: All 7 tests pass in 3.07s ✅
This level of test quality is exactly what's needed for enterprise-grade medical device software.
❌ 2 Tests Failed:
View the full list of 2 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
|




No description provided.