feat: composable requirements #91
Conversation
| class NegativeRequirement(Requirement): | ||
| def __init__(self, requirement: Requirement,): | ||
| self.requirement = requirement | ||
|
|
There was a problem hiding this comment.
This is kind of a weird thing to prompt with. But it's hard to know what a reasonable prompt would be here.
There was a problem hiding this comment.
We discussed this earlier offline but just for the historical note:
this is useful for XOR-type requirement which is sometimes natural to human.
In a different sentence like “Bob drank coffee or tea this morning”, the dominant interpretation is that he drank one or the other, but not both (i.e. exclusive disjunction annotated as XOR). https://pmc.ncbi.nlm.nih.gov/articles/PMC12039866/
Xor expands to (P and not Q) or (not P and Q).
| class ConjunctiveRequirement(Requirement): | ||
| def __init__(self, requirements: list[Requirement],): | ||
| self.requirements = requirements | ||
|
|
||
| @property | ||
| def description(self): | ||
| return "\n* ".join( | ||
| ["Satisfy all of these requirements:"] + \ | ||
| [r.description for r in self.requirements]) | ||
|
|
||
| def validate(self, *args, **kwargs): | ||
| results = [r.validate(*args, **kwargs) for r in self.requirements] | ||
| return ValidationResult( | ||
| result = all(results), | ||
| reason = "\n* ".join( | ||
| ["These requirements are not satisfied:"]+ | ||
| [r.reason for r in results if not r]), | ||
| score = max([r.score for r in results if not r])) |
There was a problem hiding this comment.
In the rest of Mellea, a list of requirements is implicitly conjunctive. So does it make sense to have a ConjunctiveRequirement that behaves differently from a list of reuqirements? Or should we explicitly not have ConjunctiveRequirements because the correct way of doing this is to pass both into the requirements= kwarg?
There was a problem hiding this comment.
List as conjunction by default is a good user interface but I believe internally they should be immediately converted into a conjunction.
In a much more complex industrial use case where a company has a large set of rules, requirement as a list becomes unmanageable. I.e., compiling a NNF into CNF causes an exponential increase in the number of clauses.
| @property | ||
| def description(self): |
There was a problem hiding this comment.
description is NOT a @property on the base class or in other Requirement types. Maybe it should be, but we should do this the same everywhere. Using property here but not in the other Requirement classes seems like a recipe for confusion down the road.
There was a problem hiding this comment.
self.description = description
mellea/mellea/stdlib/requirement.py
Line 95 in 024d9a6
description is clearly a property in the base class. wdym?
There was a problem hiding this comment.
I just mean that if we're going to use property() to handle Requirement's description, then we should do so consistently through-out the code base.
There was a problem hiding this comment.
the use of property() here is explicitly to avoid insertion, which causes an inconsistency between the parent and the child requirement.
|
There are failing quality checks. Please use the pre-commit hooks. Assuming you have already created a venv and installed Mellea (editable), you can install the hooks by running the following commands in the root of your mellea checkout: uv pip install -e . --group dev && pre-commit installOnce installed, you can still commit over errors using the |
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Enforce conventional commitWonderful, this rule succeeded.Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
guicho271828
force-pushed
the
composable-requirements
branch
from
024d9a6 to
80b83fd
Compare
2 participants
work in progress sketch for discussion
#22