solver: remove internal errors in the solver

[alalazo]

internal_error facts are parsed from concretize.lp, which happens hundreds of times during unit tests. Since they don't improve the error message substantially, just remove them to get faster CI.

Results on:

• Spack: 1.2.0.dev0 (d36d9c6)
• Builtin repo: spack/spack-packages@7134022
• Python: 3.13.2
• Platform: linux-ubuntu20.04-icelake

Before

After

[haampie]

Not noticeable in github actions

[alalazo]

@becker33 Are you fine with this? Internal errors are parsed from concretize.lp so many times during unit tests... I would be for removing them, since the error message is unintelligible to users anyhow, e.g.:

"Only nodes and virtual_nodes can have version satisfaction"

[becker33]

I really don't think this is a good idea. The error messages aren't good, but they're the only thing that tells us anything about which constraint is being violated. The first thing I do with a concretizer error that doesn't report one is make sure to add more of them until I start seeing one of them, and then I know where to start debugging.

[alalazo]

The first thing I do with a concretizer error that doesn't report one is make sure to add more of them until I start seeing one of them, and then I know where to start debugging

You can also add error facts if you're debugging. I don't see a lot of differences between:

:- <constraint>, internal_error("<debug msg>").

and:

error(10, "<debug msg>") :- <constraint>.

In any case, concretize.lp is parsed a lot of times during unit tests, and we should avoid that. I guess you also agree that for users, these messages are equally bad as a fatal error, right?

[becker33]

Yeah I agree these errors don't tell a user anything useful. But I disagree that it makes them useless. It's much better to give the user "something bad happened, here's the string to give the developers so they can figure it out" than to give them "nope, bad things happened. go away". But that's besides the point, I think those strings are really there for us to help with debugging and I'm surprised other folks don't find them essential to that effort.

[alalazo]

I think those strings are really there for us to help with debugging and I'm surprised other folks don't find them essential to that effort.

Never used internal errors to debug ASP, to be honest.

Also, I'm not suggesting to tell users to go away 🙂 I'm just saying that between:

$ spack solve zlib-ng
==> Error: Spack concretizer internal error. Please submit a bug report at https://github.com/spack/spack and include the command and environment if applicable.

and:

$ spack solve zlib-ng
==> Error: Spack concretizer internal error. Please submit a bug report and include the command, environment if applicable and the following error message.
    zlib-ng is unsatisfiable, errors are:
    internal_error("Only nodes and virtual_nodes can have version satisfaction")

there isn't a lot of added value for users.

In any case we should avoid parsing concretize.lp over and over during unit tests. Am I the only one seeing the 30s speed-up when running the tests locally, like in the description?

[alalazo]

I'll submit a competing PR to cache the symbols, but I'd like to get rid of this part of the code - it's also one of the things that are different from the solve done within Spack and outside of Spack.

[haampie]

I think those strings are really there for us to help with debugging and I'm surprised other folks don't find them essential to that effort.

Am I the only one seeing the 30s speed-up when running the tests locally, like in the description?

I'm getting a 10% speedup of unit tests locally. That's saving me more time than those internal errors when troubleshooting concretization bug reports. To me the time saved from day to day is more important.

[alalazo]

I just re-run unit tests locally against 6a44ce5 using:

$ pytest -n 8 --dist=loadfile

develop

3 failed, 5839 passed, 128 skipped, 3 xfailed, 468 warnings in 359.93s (0:05:59) 

This PR

3 failed, 5839 passed, 128 skipped, 3 xfailed, 468 warnings in 288.13s (0:04:48)

So, for me locally it's runtime reduced by 20% on this specific pair of runs.¹

Footnotes

1. The 3 failed unit tests are expected in elf.py given the first gcc I have in PATH. ↩

[becker33]

There are several methods on the Result class that should be removed as part of this PR.

• format_cores
• format_minimal_cores
• minimal_cores
• minimize_core
• format_core

We should also remove the assumptions field from the SpackSolverSetup class, and the assumptions key from the solve_kwargs dict.

We can probably also remove the cores and control fields from the Result class, as well as all the code for managing and setting them.

[alalazo]

@becker33 Done. Ready for a re-review.