Refactor ProblemGraph creation #3
Conversation
|
@syslaila, here it was it would look like. What do you think? For testing the constructor, we'll have to build a local repo with some packages, the way we did it for PubGrub. This is currently compiling. I'm happy to write some tests if we agree on this. |
This sounds good! 👍 |
| // These functions do not return a reference so we make sure to | ||
| // compute the value only once. | ||
| // TODO change name of these functions to make explicit it is not a ref. | ||
| auto source = problem.source(); |
There was a problem hiding this comment.
I noticed in cases of a conflict for a "mamba install" command that these would sometimes segfault if the id is something invalid - we might need to test this again
There was a problem hiding this comment.
Good catch! problem.source() is an std::optional so if we can pinpoint a bug we should fix it in SolverProblem.
There was a problem hiding this comment.
@syslaila if you have a reproducible example, I'm willing to investigate. Best would be if you can reproduce using libmamba/libmambapy from main and open a separate issue for it.
There was a problem hiding this comment.
I'll get an example today. However, I think we shouldn't block the merging of this PR on this since we don't support yet an "UPDATE" solver rule (and I think we would also need to filter out the ids from https://github.com/syslaila/mamba-fork/pull/3/files#diff-e214b9fbd2b0497c09f4b5738a8aec7bdb8f107f5c7e3415be65d12898fc91f3R267)
|
TODO: Actually the |
| auto src_id | ||
| = ensure_solvable(problem.source_id, std::move(source).value(), type); | ||
| auto tgt_id | ||
| = ensure_solvable(problem.target_id, std::move(target).value(), type); |
There was a problem hiding this comment.
Even though the type would be optional in this case - from https://github.com/syslaila/mamba-fork/pull/3/files#diff-e214b9fbd2b0497c09f4b5738a8aec7bdb8f107f5c7e3415be65d12898fc91f3R234. I wouldn't pass it here.
I would expect the problem_type to be different for the source package & the target package. Same in the other cases as well.
| // These functions do not return a reference so we make sure to | ||
| // compute the value only once. | ||
| // TODO change name of these functions to make explicit it is not a ref. | ||
| auto source = problem.source(); |
There was a problem hiding this comment.
I'll get an example today. However, I think we shouldn't block the merging of this PR on this since we don't support yet an "UPDATE" solver rule (and I think we would also need to filter out the ids from https://github.com/syslaila/mamba-fork/pull/3/files#diff-e214b9fbd2b0497c09f4b5738a8aec7bdb8f107f5c7e3415be65d12898fc91f3R267)
AntoinePrv
force-pushed
the
cr/problems-graph-creator
branch
from
f8b5923
to
f373656
Compare
|
@syslaila This is in a pretty good state structure-wise for you to look at. We have some tests running, but we are however printing warnings for unexpected optionals. I am not so convinced with the approach consisting of having a large I'm yet to add the |
AntoinePrv
force-pushed
the
cr/problems-graph-creator
branch
from
8965417
to
22bfaf0
Compare
| return std::nullopt; | ||
| } | ||
|
|
||
| class ProblemsGraphCreator |
There was a problem hiding this comment.
don't we need to declare it in the header file in order to use it ?
There was a problem hiding this comment.
No, because we do not use it outside satisfiability_error.cpp. It's an implementation detail only used inside ProblemsGraph::from_solver but nowhere else.
| auto ProblemsGraphCreator::add_conflict(node_id n1, node_id n2) -> void | ||
| { | ||
| m_conflicts[n1].insert(n2); | ||
| m_conflicts[n2].insert(n2); |
There was a problem hiding this comment.
| m_conflicts[n2].insert(n2); | |
| m_conflicts[n2].insert(n1); |
| auto parse_problems() -> void; | ||
| }; | ||
|
|
||
| auto ProblemsGraphCreator::ensure_solvable(SolvId solv_id, node_t&& node) -> node_id |
There was a problem hiding this comment.
the naming is confusing to me, I would use something like get_or_create_solvable but up to you
|
This looks great, thanks Antoine!
I think we can switch this to a debug messages ?
We can try it since you've done the proof of concept using the generalised approach. But yes, I think we need to do some filtering (we might also get extra packages from the solverRuleInfo eg. for solver_rule_pkg & solver_rule_job). |
I think we should get rid of all messages by fixing the code, otherwise it means our checks are not sync with what we get in practice. I haven't looked in details into the graph but perhaps it is missing some nodes.
I'll give it a go, see if does a better job.
Although I expect we'll find more issues down the road, we should fix all possible errors. I'll look into the graph see if there are missing nodes (not super convenient to do in C++ :( ) |
Hi @syslaila,
Here are some suggestions for mamba-org#1891. In the first commit, you'll find a proposal for a simplification of the ProblemGraph. I also included all utility classes inside the class to avoid saturating the
mambanamespace.In the last commit, I added a templated constructor (
from_problems) that could be used in tests with generic data. I added another constructor (from_solver) that use it with theMSolverandMPool.This is still a work in progress (everything is actually), but I think it provides good separation of concerns and encapsulation.
It does split the logic off adding problems and their dependencies in two separate functions in order to avoid a big template mess.
I don't yet have full visibility on whether it will work for all the logic we have (maybe some checks will be duplicated, hopefully not too many), but I wanted to share the idea. What do you think? I'll keep working on it tomorrow.