Refactor `cabal-install` solver config log output #9159

yvan-sraka · 2023-08-03T15:31:34Z

This PR does not modify cabal behaviour and is about code refactoring. It's part of the initiative described in this RFC #8939.

Special thanks to @andreabedini for refactoring and authoring the displayMessage' function. This significantly enhanced the readability of Distribution.Solver.Modular.Message. Moreover, the pair-programming sessions were invaluable in helping me kick off the implementation of a PATCH to address this issue :)

Checklist:

Patches conform to the coding conventions.

michaelpj · 2023-08-04T09:47:47Z

Can you show us some examples?

yvan-sraka · 2023-08-29T07:19:35Z

I've reworked the commit message a bit:

[WIP] Fix #4251: Simplify verbose "rejecting" message in solver

This commit makes the following changes:

- Enhancements to the codebase:
  * Adds several TODO/FIXME/HELP comments, laying the groundwork for
    improved solver output as described in issue #8939 ;
  * Refactors the `showMessages` function to split the logic of
    building the output (now as a `Message'` enumeration) and the
    string representation of it (now formatted by `displayMessage'`).

- Modifications to the solver output:
  * If the `-v3` or `--minimize-conflict-set` flags are not set, it
    now prompts the user to consider using them in cases of the
    "Could not resolve dependencies: ..." error ;
  * The message "(has the same characteristics that caused the
    previous version to fail: ...)" has been rephrased to
    "all other available packages. They are excluded due to the same
    constraint that caused the last version attempted to fail: ..." ;
  * Package lists are now grouped by name. For example, instead of
    displaying `aeson-1.0.2.1, aeson-1.0.2.0, aeson-1.0.1.0, ...`, it
    now shows `aeson: 1.0.2.1, 1.0.2.0, 1.0.1.0, ...`.

Before the changes …

Resolving dependencies...
Error: cabal: Could not resolve dependencies:
[__0] trying: hello-1.0.0.2 (user goal)
[__1] next goal: base (dependency of hello)
[__1] rejecting: base-4.16.4.0/installed-4.16.4.0 (conflict: hello => base>=5)
[__1] skipping: base-4.18.0.0, base-4.17.2.0, base-4.17.1.0, base-4.17.0.0,
base-4.16.4.0, base-4.16.3.0, base-4.16.2.0, base-4.16.1.0, base-4.16.0.0,
base-4.15.1.0, base-4.15.0.0, base-4.14.3.0, base-4.14.2.0, base-4.14.1.0,
base-4.14.0.0, base-4.13.0.0, base-4.12.0.0, base-4.11.1.0, base-4.11.0.0,
base-4.10.1.0, base-4.10.0.0, base-4.9.1.0, base-4.9.0.0, base-4.8.2.0,
base-4.8.1.0, base-4.8.0.0, base-4.7.0.2, base-4.7.0.1, base-4.7.0.0,
base-4.6.0.1, base-4.6.0.0, base-4.5.1.0, base-4.5.0.0, base-4.4.1.0,
base-4.4.0.0, base-4.3.1.0, base-4.3.0.0, base-4.2.0.2, base-4.2.0.1,
base-4.2.0.0, base-4.1.0.0, base-4.0.0.0, base-3.0.3.2, base-3.0.3.1 (has the
same characteristics that caused the previous version to fail: excluded by
constraint '>=5' from 'hello')
[__1] fail (backjumping, conflict set: base, hello)
After searching the rest of the dependency tree exhaustively, these were the
goals I've had most trouble fulfilling: hello, base

… and after the changes:

Resolving dependencies...
Error: cabal: Could not resolve dependencies:
[__0] trying: hello-1.0.0.2 (user goal)
[__1] next goal: base (dependency of hello)
[__1] rejecting: base-4.16.4.0/installed: 4.16.4.0 (conflict: new package does
not match existing constraint hello => base>=5)
[__1] skipping: all other available packages. They are excluded by the same
constraint that caused the last version tried to fail:
excluded by constraint '>=5' from 'hello'
[__1] fail (backjumping, conflict set: base, hello)
For detailed error messages, please rerun with the `-v3' flag.
To improve the solver output, consider running with the
`--minimize-conflict-set' option.
After searching the rest of the dependency tree exhaustively, these were the
goals I've had most trouble fulfilling: hello, base

There are many open questions left as code comments. I would greatly appreciate any feedback on them :)

michaelpj · 2023-08-29T08:09:07Z

all other available packages

Could this perhaps say "all other available versions of package "? We're not rejecting other packages and now the message doesn't tell us which package it is talking about, so when it later references a constraint it is unclear what it refers to.

michaelpj · 2023-08-30T08:34:33Z

Also, if we run with higher verbosity we should perhaps go back to listing all the package versions that get rejected.

Alternatively, I wonder if we could do something more like footnotes, e.g. with verbose output produce

Resolving dependencies...
Error: cabal: Could not resolve dependencies:
[__0] trying: hello-1.0.0.2 (user goal)
[__1] next goal: base (dependency of hello)
[__1] rejecting: base-4.16.4.0/installed: 4.16.4.0 (conflict: new package does
not match existing constraint hello => base>=5)
[__1] skipping: all other available packages (1). They are excluded by the same
constraint that caused the last version tried to fail:
excluded by constraint '>=5' from 'hello'
[__1] fail (backjumping, conflict set: base, hello)

(1): base-X, base-Y, ...

Or maybe we even print this on normal verbosity (i.e. max of two additional package versions, then an ellipsis) and then print the full list only with higher verbosity.

grayjay · 2023-08-30T08:39:31Z

Thanks for working on the solver error messages. I haven't had time to read the code carefully yet, but I have a few comments.

It looks like this PR combines multiple independent improvements, so I think it would be better to split them into separate PRs. It would also help to split the displayMessage' refactoring into its own commit or PR that doesn't change behavior.
I think that we should add a dedicated flag for showing better solver output (if it's too expensive to show it by default), rather than suggest that users rerun with -v3. -v3 shows maximum detail for debugging, such as multiple runs of the solver with --minimize-conflict-set, which isn't always helpful for users that are trying to understand conflicts.
I'm not sure we should change the logic for suggesting --minimize-conflict-set. It is very unlikely to improve the solver output for small conflict sets.
I'm not sure I understand the change to the message "has the same characteristics that caused the previous version to fail". The new message seems to be much more specific, so I don't think that it is correct in all cases. Maybe this PR should add a special case for when one version of a package is rejected by an existing version constraint, and then one or more of the following versions are rejected by the same constraint, as in the example with base above. That is a very common case, so I think that it is worth giving it a better message.

cabal-install-solver/src/Distribution/Solver/Modular/Message.hs

yvan-sraka · 2023-10-23T10:19:16Z

I've reworked this PR a bit: It now aims to add a --json flag (though I'm uncertain about the name) to the solver configuration arguments (this is the last part currently not implemented). This allows the cabal-install solver's log output to be mechanized via an external command, e.g., similar to the tree view that nix-output-monitor provides for nix-build --log-format internal-json -v.

It would be greatly appreciated if this PR could receive another round of reviews, as it significantly refactors the solver logging logic: Instead of relying on strings, it introduces a new data structure representation of logs that's utilized in several places in the code before being serialized with show.

I'm seeking advice on:

what's the best method to serialize this data structure into JSON (should I just use aeson?)
how to introduce a new user-facing solver configuration parameter in the CLI without having to modify a significant amount of code…

andreabedini · 2023-10-24T02:55:16Z

There are TODOs and clean ups left to do, but I think this is looking great. The solver is now giving structured information (SolverTrace) and cabal can take that information and give it to the user in the desired format.

SolverTrace has also a Show instance that replicates exactly how the solver was rendering its trace before so this is a drop-in replacement that should not cause any unintended change.

@yvan-sraka

what's the best method to serialize this data structure into JSON (should I just use aeson?)

for serialising JSON we already have Distribution.Client.Utils.Json.

how to introduce a new user-facing solver configuration parameter in the CLI without having to modify a significant amount of code…

Right, this hits the legacy parsing path for ProjectConfig but we can hope be able to avoid the worst of it. I'd start by following another solver flag like --strong-flag and adding this --solver-log-format (WDYT? mimicking nix's flag name) next to it. If I am not mistaken, this gets parsed in Client.Setup and converted to ProjectConfigShared by convertLegacyAllPackageFlags. The same flag has to be added to ProjectConfigShared as well, again following what is done for the other ones.

I hope this helps 🤞

grayjay

I read the updated diff, and I'm not sure I completely understand the PR. It looks like it currently has two main parts, the refactoring to add an intermediate representation of the solver log and a --json flag.

--json flag

What is the reason for formatting the log as JSON? Is it related to making the log collapsible as in #9199? I'm still not sure I understand the use case for collapsing it. Do you want to collapse the error message that is shown by default when the solver fails to find a solution, or the full verbose (-v3) log? Do you have an example of a build where it would have helped? I'm also curious to see an example because if a subtree seems irrelevant, it's possible that the solver didn't need to explore it, and it could be optimized away.

Refactoring

I can see how the SolverTrace data structure is useful for implementing different types of formatting, such as JSON or bolded output, but I was wondering if you were also planning to use it for other parts of #8939. If so, how would SolverTrace be used to change the contents of error messages?

This is the way that I understand the current code for creating the solver log:

The solver traverses the search tree and converts it to a list of Message in the backjumpAndExplore function. The list of Message aims to preserve as much of the tree structure as possible, without using a large amount of memory, by describing going up and down the branches of the tree. Then the solver uses the remaining tree structure to convert the Messages into strings that are more helpful for the user in showMessages, for example, by combining similar rejected versions into a single line.

I would expect that it would be better to apply most processing of the contents of the log to the list of Messages, where more of the tree structure is available.

yvan-sraka · 2023-11-23T11:09:31Z

To answer @grayjay's concerns, I have decided to split this PR into two parts. This PR will now focus solely on the refactoring and is no longer in draft status (I have also updated the PR description accordingly).

The other PR, which is still a draft #9465, will introduce the --solver-log-format internal-json -v option and thus modify the cabal-install CLI behavior. I plan to continue the design discussions there.

Since I feel that part of the answers to your questions was in my comments: #9159 (comment) and #9199 (comment). I wrote up in the original RFC #8939 (comment) an up-to-date plan of action of what I’m trying to achieve. Hope the whole thing makes more sense :)

grayjay

@yvan-sraka Thanks for splitting the PR. I did a full review after our discussion in the meeting.

cabal-install-solver/src/Distribution/Solver/Modular.hs

grayjay · 2023-11-28T21:32:01Z

cabal-install-solver/src/Distribution/Solver/Modular/Message.hs

@@ -41,51 +42,130 @@ data Message =
  | Success
  | Failure ConflictSet FailReason

+data Log


Could Log, SolverTrace, and related types go in cabal-install-solver/src/Distribution/Solver/Types/ with the other types that are part of the solver API?

I meant that the types related to SolverTrace/SummarizedMessage should go in the Types directory, because they are now part of the solver's API. Message should stay in Message.hs, in the Modular directory, because it is still only used internally.

cabal-install-solver/src/Distribution/Solver/Modular/Message.hs

grayjay · 2023-11-28T21:44:14Z

cabal-install-solver/src/Distribution/Solver/Modular.hs

-    createErrorMsg failure@(ExhaustiveSearch cs cm) =
+    handleFailure :: SolverFailure
+                   -> RetryLog SolverTrace String (Assignment, RevDepMap)
+    handleFailure failure@(ExhaustiveSearch cs _cm) =


Why was cm renamed?

cabal-install-solver/src/Distribution/Solver/Modular/Message.hs

grayjay · 2023-11-30T01:15:10Z

cabal-install-solver/src/Distribution/Solver/Modular/Message.hs

+  | RejectS QSN Bool ConflictSet FailReason
+  | Skipping' (Set CS.Conflict)
+  | TryingF QFN Bool
+  | TryingP QPN POption (Maybe (GoalReason QPN))


Would it make sense to split the TryingP constructor into two, one for a new goal, and one for an existing goal? Then the Maybe could be removed.

cabal-install-solver/src/Distribution/Solver/Modular/Message.hs

grayjay · 2023-11-30T03:56:04Z

cabal-install-solver/src/Distribution/Solver/Modular/Message.hs

@@ -41,51 +42,130 @@ data Message =
  | Success
  | Failure ConflictSet FailReason

+data Log


It would help to have some comments describing how the new types and Message relate to each other.

yvan-sraka · 2023-12-19T11:54:48Z

I moved the fix to #4251 into #9541 (so the name of this branch is no longer accurate, but anyway that's best to keep the conversation here focus on the refactoring)! Thanks again @grayjay for your full review, I've tried to address most of your comments :)

grayjay

@yvan-sraka Thanks! I still had some questions and comments from my last review, but they were collapsed due to the renaming changes, so I marked them with new comments to make them visible again. I also added a few comments on the new changes.

grayjay · 2023-12-22T04:14:16Z

cabal-install/src/Distribution/Client/Dependency.hs

+  ( maximumBy
+  )
+import qualified Data.Map as Map
+import qualified Data.Set as Set


Could you please undo the formatting of the imports or move it to a separate PR? It is difficult to see what has changed with the reordering.

grayjay · 2023-12-22T06:20:06Z

cabal-install-solver/src/Distribution/Solver/Modular.hs

-    createErrorMsg failure@(ExhaustiveSearch cs cm) =
+    handleFailure :: SolverFailure
+                   -> RetryLog SolverTrace String (Assignment, RevDepMap)
+    handleFailure failure@(ExhaustiveSearch cs _cm) =


grayjay · 2023-12-22T06:20:39Z

cabal-install-solver/src/Distribution/Solver/Modular.hs

 tryToMinimizeConflictSet runSolver sc cs cm =
-    foldl (\r v -> retryNoSolution r $ tryToRemoveOneVar v)
+    foldl (\r v -> retryMap mkErrorMsg $ retryNoSolution (retryMap show r) $ tryToRemoveOneVar v)


grayjay · 2023-12-22T06:20:57Z

cabal-install-solver/src/Distribution/Solver/Modular.hs

+    retryMap :: (t -> step) -> RetryLog t fail done -> RetryLog step fail done
+    retryMap f l = fromProgress $ (\p -> foldProgress (\x xs -> Step (f x) xs) Fail Done p) $ toProgress l


grayjay · 2023-12-22T06:21:49Z

cabal-install-solver/src/Distribution/Solver/Modular/Message.hs

+    -- 'Skip' should always be handled by 'goPSkip' in the case above.
+    go !l (Step (Skip conflicts)         ms) = Step (SolverTrace $ AtLevel l $ (Skipping' conflicts)) (go l ms)


grayjay · 2023-12-22T08:34:00Z

cabal-install-solver/src/Distribution/Solver/Types/Progress.hs

+data Entry
+  = LogPackageGoal QPN QGoalReason
+  | LogRejectF QFN Bool ConflictSet FailReason
+  | LogRejectS QSN Bool ConflictSet FailReason
+  | LogSkipping (Set CS.Conflict)
+  | LogTryingF QFN Bool
+  | LogTryingP QPN POption (Maybe (GoalReason QPN))
+  | LogTryingS QSN Bool
+  | LogRejectMany QPN [POption] ConflictSet FailReason
+  | LogSkipMany QPN [POption] (Set CS.Conflict)
+  | LogUnknownPackage QPN (GoalReason QPN)
+  | LogSuccessMsg
+  | LogFailureMsg ConflictSet FailReason


Shouldn't the constructor names start with "Entry" now?

grayjay · 2023-12-22T08:37:02Z

cabal-install-solver/src/Distribution/Solver/Types/Progress.hs

+  | LogSuccessMsg
+  | LogFailureMsg ConflictSet FailReason


I think that LogSuccessMsg and LogFailureMsg could be shortened to LogSuccess and LogFailure now.

grayjay · 2023-12-22T08:38:43Z

cabal-install-solver/src/Distribution/Solver/Types/Progress.hs

+  | LogSuccessMsg
+  | LogFailureMsg ConflictSet FailReason
+
+data EntryMsg = AtLevel Int Entry


It would probably be clearer to use the same name for the type and the constructor, such as EntryAtLevel.

grayjay · 2023-12-23T06:21:28Z

cabal-install-solver/src/Distribution/Solver/Modular/Message.hs

+displayMessage (LogSkipMany _ _ cs) = "skipping: " ++ showConflicts cs
+displayMessage (LogRejectMany qpn is c fr) = "rejecting: " ++ L.intercalate ", " (map (showQPNPOpt qpn) (reverse is)) ++ showFR c fr
+
+-- | Transforms the structured message type to actual messages (SummarizedMsg s).


The type name should be SummarizedMessage.

grayjay · 2023-12-23T06:28:49Z

cabal-install-solver/src/Distribution/Solver/Modular/Message.hs


 module Distribution.Solver.Modular.Message (
    Message(..),
-    showMessages
+    SummarizedMessage(..),


I don't think that SummarizedMessage needs to be re-exported from this module.

yvan-sraka force-pushed the fix-4251 branch from 8ae2193 to 84700a2 Compare August 29, 2023 07:12

angerman assigned yvan-sraka Aug 30, 2023

Kleidukos added the cabal-install: solver label Aug 31, 2023

andreabedini reviewed Sep 14, 2023

View reviewed changes

cabal-install-solver/src/Distribution/Solver/Modular/Message.hs Show resolved Hide resolved

andreabedini reviewed Sep 14, 2023

View reviewed changes

cabal-install-solver/src/Distribution/Solver/Modular/Message.hs Outdated Show resolved Hide resolved

andreabedini reviewed Sep 14, 2023

View reviewed changes

cabal-install-solver/src/Distribution/Solver/Modular/Message.hs Outdated Show resolved Hide resolved

yvan-sraka force-pushed the fix-4251 branch 4 times, most recently from 6c95e3b to 52f087e Compare October 17, 2023 06:38

yvan-sraka force-pushed the fix-4251 branch from 52f087e to a564348 Compare October 23, 2023 09:56

yvan-sraka changed the title ~~[WIP] Fix #4251: Solver "rejecting" message is too verbose~~ [WIP] Refactor cabal-install solver config log output (and add a --json machine-readable option) Oct 23, 2023

grayjay reviewed Oct 31, 2023

View reviewed changes

yvan-sraka force-pushed the fix-4251 branch from a564348 to 11ae2f8 Compare November 23, 2023 10:56

yvan-sraka marked this pull request as ready for review November 23, 2023 11:00

yvan-sraka changed the title ~~[WIP] Refactor cabal-install solver config log output (and add a --json machine-readable option)~~ Refactor cabal-install solver config log output Nov 23, 2023

yvan-sraka mentioned this pull request Nov 23, 2023

[WIP] Add a --solver-log-format internal-json option that make cabal-install solver config log output machine-readable #9465

Draft

4 tasks

yvan-sraka mentioned this pull request Nov 23, 2023

Improving Cabal Solver Output for Better Readability and Usability #8939

Open

grayjay reviewed Nov 30, 2023

View reviewed changes

yvan-sraka force-pushed the fix-4251 branch from 11ae2f8 to 913cd7d Compare December 5, 2023 21:43

Refactor cabal-install solver config log output

b20ea53

yvan-sraka force-pushed the fix-4251 branch from 913cd7d to 7bf3c33 Compare December 19, 2023 11:35

Apply some of @grayjay and @mpickering comments

f10dbcf

yvan-sraka force-pushed the fix-4251 branch from 7bf3c33 to f10dbcf Compare December 19, 2023 11:41

grayjay requested changes Dec 23, 2023

View reviewed changes

grayjay mentioned this pull request Jan 3, 2024

Add a format rejections function #9560

Merged

5 tasks

grayjay self-requested a review January 4, 2024 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `cabal-install` solver config log output #9159

Refactor `cabal-install` solver config log output #9159

yvan-sraka commented Aug 3, 2023 •

edited

Loading

michaelpj commented Aug 4, 2023

yvan-sraka commented Aug 29, 2023

michaelpj commented Aug 29, 2023

michaelpj commented Aug 30, 2023

grayjay commented Aug 30, 2023

yvan-sraka commented Oct 23, 2023

andreabedini commented Oct 24, 2023

grayjay left a comment

yvan-sraka commented Nov 23, 2023 •

edited

Loading

grayjay left a comment

grayjay Nov 28, 2023

grayjay Dec 22, 2023

grayjay Nov 28, 2023

grayjay Dec 22, 2023

grayjay Nov 30, 2023

grayjay Dec 22, 2023

grayjay Nov 30, 2023

grayjay Dec 22, 2023

yvan-sraka commented Dec 19, 2023

grayjay left a comment

grayjay Dec 22, 2023

grayjay Dec 22, 2023

grayjay Dec 22, 2023

grayjay Dec 22, 2023

grayjay Dec 22, 2023

grayjay Dec 22, 2023

grayjay Dec 22, 2023

grayjay Dec 22, 2023

grayjay Dec 23, 2023

grayjay Dec 23, 2023

		retryMap :: (t -> step) -> RetryLog t fail done -> RetryLog step fail done
		retryMap f l = fromProgress $ (\p -> foldProgress (\x xs -> Step (f x) xs) Fail Done p) $ toProgress l

		-- 'Skip' should always be handled by 'goPSkip' in the case above.
		go !l (Step (Skip conflicts) ms) = Step (SolverTrace $ AtLevel l $ (Skipping' conflicts)) (go l ms)

Refactor cabal-install solver config log output #9159

Are you sure you want to change the base?

Refactor cabal-install solver config log output #9159

Conversation

yvan-sraka commented Aug 3, 2023 • edited Loading

michaelpj commented Aug 4, 2023

yvan-sraka commented Aug 29, 2023

michaelpj commented Aug 29, 2023

michaelpj commented Aug 30, 2023

grayjay commented Aug 30, 2023

yvan-sraka commented Oct 23, 2023

andreabedini commented Oct 24, 2023

grayjay left a comment

Choose a reason for hiding this comment

--json flag

Refactoring

yvan-sraka commented Nov 23, 2023 • edited Loading

grayjay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yvan-sraka commented Dec 19, 2023

grayjay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Refactor `cabal-install` solver config log output #9159

Refactor `cabal-install` solver config log output #9159

yvan-sraka commented Aug 3, 2023 •

edited

Loading

yvan-sraka commented Nov 23, 2023 •

edited

Loading