core: (Constraints) more complex AnyOf simplification#6084
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6084 +/- ##
=======================================
Coverage 86.86% 86.87%
=======================================
Files 428 429 +1
Lines 64264 64311 +47
Branches 7374 7383 +9
=======================================
+ Hits 55825 55870 +45
- Misses 6874 6875 +1
- Partials 1565 1566 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| return | ||
| raise VerifyException(f"Unexpected attribute {attr}") | ||
|
|
||
| def __or__( |
There was a problem hiding this comment.
why the rename? is it because you'd like to make merge variadic later? I feel like using | is pretty
There was a problem hiding this comment.
It's not a renaming, | is still there. merge is different because it can fail.
There was a problem hiding this comment.
Oh, then I feel like it would be nice to add more to the doc string as motivation, as I'm not sure that I get the purpose. Is the idea for | to use the merged output if possible and fall back to AnyOf if that fails? Isn't this kinda what already happens in the code?
There was a problem hiding this comment.
the return below could just return super.__or__(self, other) instead, right?
There was a problem hiding this comment.
The purpose is purely to merge ParamAttrConstraints together in an AnyOf. I agree that's not obvious from the naming and will happily take suggestions. I actively don't want to fall back to AnyOf here.
There was a problem hiding this comment.
The alternative is to inline the logic into the loop of AnyOf.get but pyright was getting really angry with ParamAttrConstraint[Unknown]
There was a problem hiding this comment.
OK sorry let me actually properly read the code now
There was a problem hiding this comment.
OK I think I get it now. I would still like to avoid this merge method if possible, although I now understand its purpose better. I'll have a think about it as currently I have no good suggestions. My gut tells me that we might be able to leverage the init better, or maybe rethink the init of this constraint (and the internal storage) A part of the idea here is whether the AnyOf is actually just an AnyOf of length one, right? Couldn't we construct the AnyOf, and then check whether it could actually be represented by a single constraint? I'm not 100% sure of the details of how this would work, curious if you think it's promising.
There was a problem hiding this comment.
I'm not sure this is related to the length 1 thing. The idea is that you have a list of constraints, and some of these may be "mergable" ParamAttrConstraints (i.e. same base and have at most 1 differing parameter constraint)
AnyOf.get then goes through its constraints and for each constraint, it tries to "merge" it with all the constraints it's seen before. If the merge succeeds, we replace both constraints with the merged one, and if not we leave both in the AnyOf.
I think I've maybe made things messier by combining this with unrolling nested AnyOf's. In any case I don't like the code I've written so definitely need to polish it up.
|
@superlopuh should I split this up? |
|
Sorry I thought you were planning to play with this some more. I haven't really had the mental bandwidth to look at this properly, if you're not blocked on this then having another few days to take a look would be great. |
|
I think I'll split off the easier bit anyway, because that can't be a bad thing. I'm not blocked on any of this, but I did promise to get the ParamAttrConstraint merging in |
|
Just to double check, this .get is fully addressing this comment, right? #6056 (comment) |
|
This does implement that functionality yes. (This is what this .merge function is doing) |
|
Just thought I'd add an update here. I haven't had time to work on this much but my current thinking is that it might be cleaner for the main building function be something that adds a single element to an existing AnyOf. This would fit better with how we usually build these constraints as: constr1 | constr2 | constr3and I think it will be cleaner code. |
|
That would be great, in the discussion above you mentioned that this wasn't always possible. I think it might still be preferable to it not working at all for the time being. |
|
@alexarice what's your plan/timeline here? Will you have time to work on it this week or is it more of a post-CGO thing? |
|
My CGO deadline is fine. It's my POPL deadlines that I'm stressing for. I certainly won't have time this week, but could schedule some time to tackle it next week? |
|
I had a look at this and have kind of been defeated by trying to do this in a nicer way. |
|
Yeah it's tough. I'll try to take a look at this later this week, I think some way that works is better than nothing. |
|
I had hoped to remove AnyOf.attr_constrs and just make the init deal with the other three fields, but we depend on |
|
I think the idea here is ok, if we can find a better name than |
|
As per our chat on Zulip, I think |
| self, other: AttrConstraint[AttributeCovT] | ||
| ) -> AttrConstraint[AttributeCovT] | None: |
There was a problem hiding this comment.
How do you feel about this?
| self, other: AttrConstraint[AttributeCovT] | |
| ) -> AttrConstraint[AttributeCovT] | None: | |
| self, other: Self | |
| ) -> Self | None: |
There was a problem hiding this comment.
Feels like a useful restriction to me
There was a problem hiding this comment.
Also, feels like adding this separately might be useful, with a dedicated test for relaxing parametrized attribute constraints? That way we know that we're testing this functionality separately from the AnyOf stuff.
There was a problem hiding this comment.
I'm also wondering whether the default implementation should be an equality check, returning self if self == other, feels like this would be a good discussion for a focussed PR.
There was a problem hiding this comment.
I haven't looked at this code in a while but at the point we use relax_constraint we don't know if the two types are the same. I feel if we went down this route it would be better just to hardcode the logic for ParamAttrConstraint. To be honest I can't remember why I didn't do this.
There was a problem hiding this comment.
Hmm, I see what you mean. Let's forget about the Self stuff. To me it feels like it's fine as-is in this case.
There was a problem hiding this comment.
Actually I still feel like adding this in a separate PR might be worth it, just to make things a little cleaner. I would also probably add the return self if self == other else None default implementation.
There was a problem hiding this comment.
Just so that we're on the same page, I meant adding the changes highlighted in this comment in a different PR, adding the function here and on ParamAttrConstraint, adding the relevant tests, and then adding the AnyOf simplification + tests in a different PR. LMK if you feel like this kind of split doesn't make sense.
There was a problem hiding this comment.
I'm on the same page, and this makes sense, but it is not my priority at the moment.
There was a problem hiding this comment.
Sorry, forgot about POPL, not urgent on my end.
2 participants
Some more complex simplification of AnyOf constraints.
I'm not sure I'm happy with this setup with adding the extra
mergehelper function, but it could be helpful for extending this in the future if wanted.Code is a bit messy so would welcome any nitpicking