-
Notifications
You must be signed in to change notification settings - Fork 291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
evaluator: 0.5 regression with disjunctions and comprehensions #2209
Comments
Reduced from 72 lines to 25, though note that now master only gives one of the errors. If we have a potential fix, we should verify if the other errors are still present.
|
Worth noting that the workarounds that @slewiskelly mentions still appear to work, such as removing the |
Unfortunately, the fix was breaking too much other code. We already knew this was a brittle change. We now plan to fix this as part of an evaluator revamp. This has very high priority, but will not make it to v0.5, unfortunately. |
@mpvl: How should we understand the status of the evaluator rework? If I have a regression in my project when I go from v0.4.3 to v0.5.0-beta.5 similar to the ones I have reported before, should I try to put together a repro? |
@bttk - we will be publishing a plan outlining next steps for v0.5.0, v0.6.0 and v0.7.0 this week. I will link to that discussion from here. |
@slewiskelly @bttk - we will shortly be posting a discussion about how we are approaching the v0.5.0, v0.6.0 and v0.7.0 releases. Indeed we be covering this topic on the Community Call next week. |
Marking this as v0.7.0. If we were to address this properly we would likely need an involved and extensive fix as part of v0.6.0. As things stand, we don't want to delay v0.6.0 if we can avoid it, and instead prefer to address this along with other performance and disjunction issues in v0.7.0. This has the added benefit of the fix likely being considerably easier, because of refactors in the evaluation engine as part of v0.7.0 (that are not part of v0.6.0, where the cost of making such changes is higher). |
Might the fix be included in the We'd appreciate to be able to pick-up the release, as we've been very much looking forward to the performance improvements, as well as adopting some other features since |
@slewiskelly - apologies for the delay in replying here. Since your message we've been improving how we communicate about updates to the performance of CUE via #2857 and an office hours session on Thursday #2862. Next week we will check where this issue stands with regards to the new evaluator, and whether it makes sense for a fix to be applied to the old (current) evaluator. |
@slewiskelly - not forgotten this issue. Marcel is still deep in work on performance in the new evaluator (#2884), and per the updates in #2850 we are targeting releasing an experimental version of the new evaluator via #2886. That was scheduled to happen by the end of March, but we've slipped a bit on that. We'll be posting an update to #2850 today updating that we should be in a position to release an experimental version next week. At which point we will be able to return to this issue and properly evaluate whether a fix can be included to the "old" evaluator (so named, simply because we need a way of distinguishing the two). I've placed this issue in the Evaluator Roadmap as "planned" to indicate that we will look at whether it's feasible to include a fix in the v0.8 series for the regression. |
Here is an updated testscript that passes - it shows the current behavior as of 6694c55. It's worth noting that evalv3 still fails, although with a different error now.
|
What version of CUE are you using (
cue version
)?Does this issue reproduce with the latest release?
Yes.
git bisect
pinpoints to 748a685 (https://review.gerrithub.io/c/cue-lang/cue/+/529524).What did you do?
cue export
with the following configuration snippet.A similar, but larger configuration has been used for several months using
v0.4.3
. Upon evaluatingv0.5.0
, failures are encountered.Reproducer
Forgive the length of the reproducer. I tried to maintain the spirit of what is trying to be accomplished, while removing as much as possible.
What did you expect to see?
Result v0.4.3
What did you see instead?
Result v0.5.0
I can make it work with by making (at least) either of the following two changes:
Diff
#Constrained
definitionDiff
The text was updated successfully, but these errors were encountered: