244 architecture resolve resource dependencies - #245
Conversation
- Add resource module with helper to determine resources required by enabled metrics - Add helper to identify ready-to-use resources and those requiring preparation - Add resource_test module with tests for both helpers - Delete resource dependency related code from config and config test modules - Move qc_config_factory fixture to new conftest.py in quality_control test package
|
@roryclaydon1994 I am going to switch to draft mode as I am about to add a new resource orchestration helper with some tests. I will ping you when it's ready for review |
- Add helper to resolve resources required by enabled metrics - Raise an explicit error when required resources are unavailable - Add composition tests for resolved, missing, and unrequired resources
|
@roryclaydon1994 I added the new commits and updated the description. This PR is ready for review. thanks! |
roryclaydon1994
left a comment
roryclaydon1994
left a comment
There was a problem hiding this comment.
Review Feedback
Tests are very complete, good documentation and typing throughout.
In terms of design, it looks like resources.py could be condensed to one simple function or a class with method names that are easier to distinguish and could be overloaded in future if desired.
Re the Resources class my meaning is get_required_resources and resolve_required_resources become get_required/filter_enabled and resolve as the resource class already provides the fact it is related to resources.
| return { | ||
| resource | ||
| for metric in config.metrics.values() | ||
| if metric.enabled | ||
| for resource in metric.required_resources | ||
| } |
There was a problem hiding this comment.
Can a metric be disabled in config, but passed in as provided?
There was a problem hiding this comment.
Yes, a resource can be passed in as provided even if it isn't required by any enabled metric. In the migrated code, the handling of this behaviour is covered by the get_required() tests in the updated implementation. The resolve() tests separately cover filtering the caller-provided resources against the required resources. For example, when psf_models.standard is required but both standard and oversampled are provided, only standard is returned as resolved.
There was a problem hiding this comment.
What does it mean for a resource to be provided, but not required? Would it be used?
There was a problem hiding this comment.
I see this primarily as allowing a caller to provide a broader set of resources than the current configuration requires, for example when setting up shared/default resources or testing different configurations. Resources that aren't required by any enabled metric are simply ignored by resolve() and therefore aren't used by the QC pipeline.
There was a problem hiding this comment.
I'm not quite following, but I'm assuming examples will come in the next and MR, and at present, this seems like it will not cause an issue.
I think a compromise might be to add a quick debug logging call to the function to specify what was missing, what was resolved, and was provided but not required.
There was a problem hiding this comment.
okay, no p. I can add a debug logging call and will ping you when it's been committed
There was a problem hiding this comment.
pushed the new commit. No new test was added because the unused-resource scenario is already covered by the existing parameterized test.
- Define Resources class as the resource management API boundary - Move and consolidate resource helpers into class methods - Update resources unit test module
|
Thanks @roryclaydon1994 again for your thoughtful review. I followed the suggestion in your feedback and introduced a |
2 participants
Summary
Adds resource dependency helpers to identify resources required by enabled
quality metrics, determine which required resources are ready to use and
which still require preparation.
Closes #244
What’s changed
resource.pymodule with a helper to determine resources required by enabled metricsresource_test.pywith tests for both helpersquality_control.config.pyandconfig_test.pyqc_config_factoryfixture to a newconftest.pyin thequality_controltest packageHow to test / verify
Scope
Changelog
Reviewer Checklist
develop, ormainfor release PRs)ruff)Next Steps / Notes (if applicable)
Resource preparation is intentionally not implemented in this PR. The current resource module identifies missing resources so that a subsequent stage can determine how they should be prepared or supplied.