refactor: enforce act/aact without strategy - #1468
Conversation
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
markstur
left a comment
markstur
left a comment
There was a problem hiding this comment.
Before "Approve", I want to ask what "first pass at" implies. Is this ready and that's just the right way to describe continuous improvement or does it indicate work-in-progress? It sounds unfinished, but it looks good.
I found 2 nits which you may choose to ignore:
1 - see inline: I commented where you might use join() instead of list repr, but I already decided that I like your way for simple/effective.
2 - I think the very verbose exception message repeated in a comment and in a docstring is a bit much. I would try to shorten or eliminate the comment if the code is more readable w/o it (the message is near enough) -- but that's something I only recommend you do if you are in the code anyway and "see it" the way I did. Not blocking approval.
| raise ValueError( | ||
| "Requirements were provided without a SamplingStrategy and " | ||
| "are not rendered by the action, so they cannot be validated: " | ||
| f"{[r.description for r in unvalidated]}. Pass a `strategy` " |
There was a problem hiding this comment.
I was going to suggest a join() here to make a nicer natural language message, but using a list like this gets readability with less worry about quotes and separators. I'd keep as-is.
There was a problem hiding this comment.
Shortened one of the verbose comments.
Re first pass - a (bad?) habit of mine that on a first commit to a new branch.
In the spirit of continuous improvement - this commit should cover everything as is.
AngeloDanducci
changed the title
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
jakelorocco
left a comment
jakelorocco
left a comment
There was a problem hiding this comment.
one small question; thanks for updating the docstrings as well, makes things much more clear
| if requirements: | ||
| rendered = _requirements_rendered_by_action(action) | ||
| unvalidated = [r for r in requirements if id(r) not in rendered] |
There was a problem hiding this comment.
Can you please elaborate on why this is needed? Can we modify ainstruct to just not pass the requirements if no sampling strategy is provided (or whatever makes sense in that case)?
There was a problem hiding this comment.
For ainstruct yes (and maybe that is sufficient).
Calling aact(action, requirements=[loose_req], strategy=None) directly would still silently drop loose_req with no signal though IIUC.
There was a problem hiding this comment.
Okay, then I would advocate for:
- doing any necessary rendered / unrendered requirement parsing in ainstruct (which I don't think will require a function like this since all the data is there from parameters)
- just raise an exception if a requirement is passed directly into aact without a strategy (we can even investigate changing the overloads to make this more clear)
There was a problem hiding this comment.
Addressed - I agree I think this is cleaner.
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
3 participants
Pull Request
Issue
Fixes #1448
Description
Code now has two requirements set apart:
Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.