-
Notifications
You must be signed in to change notification settings - Fork 36
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
feat: merge dependencies for better error messages #163
Conversation
The merge_incompatibilities function is a bit hard to read (or maybe it's getting too late ^^). If you could re-explain the approach it would help. In my point of view, improving error messages and some worst case scenarios, at the cost of degrading the easy solutions is worth it. |
You are not wrong. I got distracted by the performance problems and forgot to clean up the code. Sorry. |
Rebased and added a commit splitting things up and added some comments. Hopefully this is clearer. |
I have a variation that does not have a dedicated map for I'm still thinking about ways to have our cake and eat it too. Something that only allocates if we are in a large case where it would be more efficient, and uses scanning when inputs are small. But I have not thought of result that is efficient in both cases and simple to implement. |
I haven't taken the time to re-read the code yet. However I've watched packaging con 2023. This talk https://youtu.be/z0AB4g2KDX8?si=tpz1DOEYdLXVmPJw about explainability of errors in spack could be relevant. One idea is that overhead can only be run in a second stage, if an error was found. This is akin to the "simplify" that was recently merged, but I guess something like this could also be done for what is proposed here. Something like follows:
With the knowledge that (3) brings I guess that second run should be fairly fast, and with simplified incompatibilities maybe it takes better shortcuts and give a better erroring path overall? |
I tested some of the larger examples I generated from cargo use cases. They do not hit this pattern. The tool I have for generating these examples is very out of date, I did not look to see if the tool could be updated to meet this pattern. This also gave me the opportunity to measure the absolute overhead of this approach, which is quite small. The relative overhead is high because Elm if the only input to have this pattern in my benchmark suite and it has very little backtracking. I'd like to hear from @zanieb or @konstin, about whether there real world use case it's this code path. If it does, we can always improve performance later. |
There is a nice speedup:
|
The error message becomes 5 lines instead of 2000 |
Are the lines unreadably long? That's my primary concern, although perhaps we can handle splitting lines into readable pieces separately. I'm making good progress on packaging scenarios for testing error messages at https://github.com/zanieb/packse |
I believe, as of this PR, the lines are still long. However, they get much shorter if @mpizenberg can / do you want to look at this before it gets merged? |
I haven't heard about long lines before. Is this something discussed in the office hours? I can have another look at this Friday if you are ok waiting two more days. Otherwise I trust you. |
I'm curious about an example for this if possible. |
I think the test added in this PR is a great example. In the before case there were
The line
Become something like
The line
Become something like
I would like to defer conversation about future improvements to future PR's/issues, so that we can focus on getting this improvement merged.
Happy to wait! If I don't get an update, I will merge on Monday. If you spot something after that open an issue and will get it fixed. |
src/internal/core.rs
Outdated
@@ -30,6 +30,8 @@ pub struct State<P: Package, VS: VersionSet, Priority: Ord + Clone> { | |||
/// and will stay that way until the next conflict and backtrack is operated. | |||
contradicted_incompatibilities: rustc_hash::FxHashSet<IncompId<P, VS>>, | |||
|
|||
dependencies: Map<(P, P), SmallVec<IncompId<P, VS>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you already tested it but I'm gonna ask just in case. Considering we only merge dependants where the dependency is exactly the same, would it make sense to store the dependency in the key? It does increase memory so it's probably worse, but it would prevent from looping through all incompats for the packages pair.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose these package pairs are generally small anyway since you opted for SmallVec
so yeah maybe it's a bad trade off. But still curious to hear from if you tried it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering we only merge dependants where the dependency is exactly the same, would it make sense to store the dependency in the key? It does increase memory so it's probably worse, but it would prevent from looping through all incompats for the packages pair.
I would absolutely love to do this. Unfortunately VS
is neither Hash
nor Ord
, so I don't know of an efficient data structure for using them as a key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, Ord
would not make sense for sets. Hash
could make sense. Maybe we can make a note of this, to explore in the future if it turns out some real-world use case end up being in the worst case scenario for this (many different ranges for a package pair, causing O(n²) complexity).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Realistically, this can only happen for a package pair where there are many versions of each, their versions are bumped in sync. Something like two packages from the same workspace maybe if versions are strictly specified. Or for things like features (in our "pubgrub limitations" guide section) where we pair exactly a feature version to the bare crate version. Hum, maybe not so unlikely ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. "Features" is why none of the cargo benchmarks hit this code path.
I'm not too afraid of the O(n²) overhead here. Every time we merge dependencies we follow it up by doing unit_propagation
, which does a linear scan over an even larger collection of incompatibilities. (Well... unit_propagation
has "previously_contradicted" as an optimization. So theoretically speaking unit_propagation
is not guaranteed to be slower, but so far it has been in practice.) That is to say it has the same O(n²) shaped, with a larger n. If this becomes a hotspot in a resolution that is slow we can investigate a fixed then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a few cosmetic changes, changing some names and comments but otherwise I've nothing to add. Looks good to me if you confirm there isn't a big performance hit.
I will rerun and annotate my usual benchmarks at some point today. |
Also looks The optimization can hit in The randomly generated
Overall I wish the performance impact was smaller, but we can always work on it in follow-up PR's. |
* bad error test * feat: merge direct dependencies * separate function for clearer documentation * Update doc comments for merge_dependency * Rename merge_dependency into merge_dependants * Use american english dependent instead of dependant * Rename dependencies into merged_dependencies * Typo mergeed -> merged --------- Co-authored-by: Matthieu Pizenberg <matthieu.pizenberg@gmail.com>
* Update test following pubgrub-rs#163 * Fix compiler warning --------- Co-authored-by: konstin <konstin@mailbox.org>
* Update test following pubgrub-rs#163 * Fix compiler warning --------- Co-authored-by: konstin <konstin@mailbox.org>
* Update test following pubgrub-rs#163 * Fix compiler warning --------- Co-authored-by: konstin <konstin@mailbox.org>
This is the "simplest thing that could possibly work", when adding a new dependency incompatibility we check to see if there is an existing incompatibility with the same parent package and dependent package and dependent VS, if so we construct a new incompatibility that represents the merging of the two incompatibilities. We remove the old incompatibility from all lookup tables and at the new one. This effectively brings us on par with Dart for #19. When it applies this can massively reduce the number of incompatibilities the algorithm scans through. So it may help a little bit with #135, depending on whether there real problem contain this exact case. The union of versions done when merging incompatibilities here is definitely a candidate for #163.
The big advantage of this is the improvement to error messages. In fact this is split up into several commits the first one as a test case with the old error message and the second one shows the new error message. Our error messages tend to have "holes" constructed by discovering the individual versions don't work. When each version doesn't work for a different reason then we are in a complicated mess and the "holes" are the least of the users problems. In this case, the only reason we discovered the issues one at a time is that we loaded the data one version at a time, which is not useful information for the user to see. I consider this improvement to error messages as justifying a lot of "half-baked" details about this approach.
Unfortunately this is pure overhead when the optimization does not apply. Most problematically this does not apply if we never load more than one version of a package, that is to say when we easily construct a solution. All told this adds a 5% to 20% cost on our benchmarks. After several days of experimenting this slowdown is caused not by any of the inefficient/half-baked things that I would like to improve. :-( Instead it is caused by constructing, populating, and dropping the additional hash table.
Can you think of a more space efficient way to find the incompatibility to merge with?
Do you think the performance cost is worth the error message improvement?
What is the performance cost on your preferred workload?