Generalise rejection policy configuration - #248
Conversation
- Replace the hard-coded threshold field with a generic policy mapping - Update RejectionPolicyConfig to support policy-specific parameters - Update parse_rejection_policy_config() - Update the YAML configuration examples and fixtures - Update rejection-policy tests - Use backticks in error messages to highlight configuration parameter names
roryclaydon1994
left a comment
roryclaydon1994
left a comment
There was a problem hiding this comment.
Review Feedback
Changes look good, consistently good documentation throughout. Design is flexible and the type enforcement is likely to limit the error space.
In terms of design, I'm wondering if the constraint of one policy per metric could be relaxed such that one pass through the data could handle multiple outputs, one per policy? Also, given that the typing is strong, perhaps reading the configs to a dataclass first to handle the type handling would give a consistent API to the configs and would allow the parsing functions to focus on the policy extraction?
|
Thanks, @roryclaydon1994 for the review! I agree that having the configuration represented as typed dataclasses as early as possible would provide a cleaner API. My current understanding is that this would require either a typed representation of the full YAML configuration before section parsing, or moving the mapping-to-dataclass conversion into the individual configuration classes. I'd like to explore that separately rather than introduce that architectural change into this PR, since the current parsers provide the boundary between the raw YAML mappings and the validated For multiple policies per metric, I think enforcing one policy for now keeps the configuration and execution model simple. If a concrete use case emerges for multiple policies, we can revisit the configuration and combine the policy outputs at that point. I will now address each individual conversation thread :-) Done! Ready for you to have a second pass |
- Raise ValueError when the policy field is missing - Extract policy from the configuration mapping - Add unit tests for missing policy and invalid policy types - Remove duplicate rejection configuration mapping test
2 participants
Summary
This PR generalises the rejection policy configuration section by replacing the hard-coded threshold field with a generic policy mapping. The current hard-coded representation limits the configuration model as additional policy types (e.g. quantile-based policies) may require different parameters.
Closes #247
What’s changed
RejectionPolicyConfigto support policy-specific parametersparse_rejection_policy_config()How to test / verify
Scope
Changelog
No changelog fragment is added because the final user-facing configuration structure is still being developed. The changelog entry will be added with the PR that finalises the configuration structure.
Reviewer Checklist
develop, ormainfor release PRs)ruff)Next Steps / Notes (if applicable)
Continue development of the pipeline orchestration PR, incorporating the generalised rejection policy configuration into rejection policy instantiation.