feat: pre built requirements#606
feat: pre built requirements#606akihikokuroda wants to merge 4 commits intogenerative-computing:mainfrom
Conversation
|
The PR description has been updated. Please fill out the template for your PR to be reviewed. |
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/
|
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
akihikokuroda
force-pushed
the
pre-builtreq
branch
from
4f2f4fc to
ccabb28
Compare
|
Remaining items: Guardian intrinsics integration and are in the follow up PRs. |
It seems that already implemented. |
|
@nrfulton would this go better here or in mellea-contribs? |
| # Note: remove() uses identity (is), not equality (==) | ||
| # Since we're creating new instances, this won't find them | ||
| # This is expected behavior - remove by reference |
There was a problem hiding this comment.
does this mean that remove() doesn't work? not quite sure I follow what's going on here
There was a problem hiding this comment.
This is related to the self.copy(). For the operation functions, a copy argument is added. Remove works without the copy.
| keywords = RISK_KEYWORDS[risk] | ||
| for keyword in keywords: | ||
| if keyword in text: | ||
| detected_risks.append(risk) | ||
| break |
There was a problem hiding this comment.
if I'm understanding right, this requirement basically checks to see if specific terms are used, and if they are then it flags them as a risk. There's a major risk of false positives here... we might want to reevaluate this one.
There was a problem hiding this comment.
This is the keyword base implementation so there are limitations and it is added in the comment. "Guardian intrinsics integration" should improve it. We can add it in a separate PR.
| or the deprecated GuardianCheck class with appropriate backends. | ||
| """ | ||
| # Define harmful keywords for each risk type | ||
| RISK_KEYWORDS = { |
There was a problem hiding this comment.
should we define this top level (rather than recreating each time we call this function)?
|
|
||
| combined = safety + format | ||
| assert isinstance(combined, RequirementSet) | ||
| # Note: May have duplicates (e.g., no_pii appears in both) |
There was a problem hiding this comment.
would it be better to dedupe when composing profiles?
There was a problem hiding this comment.
To make it work, eq/hash have to be added to the base Requirement class. For now, a separate deduplicate method is added to deduplicate manually.
| raise TypeError( | ||
| f"Expected Requirement instance, got {type(requirement).__name__}" | ||
| ) | ||
| new_set = self.copy() |
There was a problem hiding this comment.
looks like all the functions here call self.copy(). do we need to make full copies each time? (that could get expensive)
There was a problem hiding this comment.
immutability prevents bugs and makes behavior predictable so a copy argument is added to the operation method not to copy optionally.
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
2 participants
Misc PR
Type of PR
Description
Testing