Skip to content

fix: Handle many missing top-level packages #4361

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.

Sign up for GitHub

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

Open
jjerphan wants to merge 1 commit into
base: main
Choose a base branch
from
+117 −21
Open

fix: Handle many missing top-level packages #4361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 76 additions & 21 deletions libmamba/src/solver/problems_graph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
#include <limits>
#include <map>
#include <sstream>
#include <stdexcept>
#include <string_view>
#include <tuple>
#include <type_traits>
Expand Down Expand Up @@ -378,6 +377,28 @@ namespace mamba::solver
return std::visit([](const auto& n) -> std::string_view { return name_or(n, ""); }, node);
}

/**
* The name of a CompressedProblemsGraph::node_t.
*/
auto compressed_node_name(const CompressedProblemsGraph::node_t& node) -> std::string_view
{
return std::visit(
[](const auto& n) -> std::string_view
{
using Node = std::decay_t<decltype(n)>;
if constexpr (std::is_same_v<Node, CompressedProblemsGraph::RootNode>)
{
return "";
}
else
{
return n.name();
}
},
node
);
}

/**
* The criteria for deciding whether to merge two nodes together.
*/
Expand Down Expand Up @@ -533,7 +554,16 @@ namespace mamba::solver
{
new_graph.add_edge(new_from, new_to, CompressedProblemsGraph::edge_t());
}
new_graph.edge(new_from, new_to).insert(old_graph.edge(old_from, old_to));
// NamedList requires a single package name. Different dependency names can
// map to the same (from, to) pair after node merges (e.g. via provides);
// keep the first name and drop incompatible MatchSpecs rather than aborting
// problem reporting.
auto& new_edge = new_graph.edge(new_from, new_to);
const auto& old_edge = old_graph.edge(old_from, old_to);
if (new_edge.empty() || (invoke_name(old_edge) == new_edge.name()))
{
new_edge.insert(old_edge);
}
};
old_graph.for_each_edge_id(add_new_edge);
}
Expand Down Expand Up @@ -649,22 +679,17 @@ namespace mamba::solver
{
if (first < last)
{
const auto first_name = std::string(invoke_name(*first));
for (auto it = first; it < last; ++it)
{
if (invoke_name(*it) != invoke_name(*first))
// Keep a single package name per list. Divergent names can appear after
// aggressive graph merges; dropping them preserves explainability.
if (invoke_name(*it) == first_name)
{
throw std::invalid_argument(
util::concat(
"iterator contains different names (",
invoke_name(*first),
", ",
invoke_name(*it)
)
);
Base::insert(*it);
}
}
}
Base::insert(first, last);
}

template <typename T, typename A>
Expand Down Expand Up @@ -799,12 +824,12 @@ namespace mamba::solver
template <typename T_>
void CompressedProblemsGraph::NamedList<T, A>::insert_impl(T_&& e)
{
// NamedList is a same-name collection. When explaining complex unsolvable
// environments, incompatible names can show up after node/edge merges; skip
// them instead of aborting the error message with an exception.
if ((size() > 0) && (invoke_name(e) != name()))
{
throw std::invalid_argument(
"Name of new element (" + invoke_name(e) + ") does not match name of list ("
+ name() + ')'
);
return;
}
Base::insert(std::forward<T_>(e));
}
Expand Down Expand Up @@ -1115,17 +1140,29 @@ namespace mamba::solver
}

// All children are the same type of visited or leaves, no grand-children,
// and same status.
// same status, and same package name.
// We dynamically delete all children and mark the whole node as such.
auto all_same_split_children = [](TreeNodeIter first, TreeNodeIter last) -> bool
// Name equality is required because NamedList / problem messaging assumes a
// single package name per node; providers of the same dependency can differ.
auto all_same_split_children = [&](TreeNodeIter first, TreeNodeIter last) -> bool
{
if (last <= first)
{
return true;
}
auto same = [&first](const TreeNode& tn)
{ return (tn.type == first->type) && (tn.status == first->status); };
return std::all_of(first, last, same);
if (!std::all_of(first, last, same))
{
return false;
}
const auto first_name = compressed_node_name(m_pbs.graph().node(children_ids.front()));
return std::all_of(
children_ids.begin(),
children_ids.end(),
[&](node_id id)
{ return compressed_node_name(m_pbs.graph().node(id)) == first_name; }
);
};
const TreeNodeIter children_end = out;
if ((n_children >= 1) && all_same_split_children(children_begin, children_end))
Expand Down Expand Up @@ -1624,7 +1661,15 @@ namespace mamba::solver
for (auto id : ids)
{
const auto& node = std::get<Node>(m_pbs.graph().node(id));
out.insert(node.begin(), node.end());
// NamedList is single-name; when a split groups providers of different names,
// only keep entries that match the first name so messaging can still proceed.
for (const auto& pkg : node)
{
if (out.empty() || (invoke_name(pkg) == out.name()))
{
out.insert(pkg);
}
}
}
return out;
}
Expand Down Expand Up @@ -1670,8 +1715,18 @@ namespace mamba::solver
{
for (auto t : to)
{
if (!m_pbs.graph().has_edge(f, t))
{
continue;
}
const auto& e = m_pbs.graph().edge(f, t);
out.insert(e.begin(), e.end());
for (const auto& ms : e)
{
if (out.empty() || (invoke_name(ms) == out.name()))
{
out.insert(ms);
}
}
}
}
return out;
Expand Down
41 changes: 41 additions & 0 deletions libmamba/tests/src/solver/test_problems_graph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,46 @@ namespace
return create_pubgrub_hard_(ctx, channel_context, true);
}

/**
* Many missing top-level packages plus some with broken transitive deps.
*
* Regression for problem-tree messaging aborting with
* "Name of new element (...) does not match name of list (...)" when explaining
* large unsatisfiable environments (e.g. win-arm64 missing conda-forge coverage).
*/
auto create_many_missing_top_level(Context&, ChannelContext& channel_context)
{
return std::pair(
create_pkgs_database(
channel_context,
std::array{
mkpkg("mitmproxy", "1.0.0", { "aioquic=1.2.0" }),
mkpkg("sphinx-book-theme", "1.0.0", { "sphinx", "theme-dep-missing" }),
mkpkg("sphinx", "1.0.0", { "requests" }),
mkpkg("sphinx-copybutton", "1.0.0", { "sphinx" }),
mkpkg("requests", "1.0.0", { "urllib3", "brotli-python" }),
mkpkg("pre-commit", "1.0.0", { "python" }),
mkpkg("python", "3.12.0"),
}
),
Request{
{},
{
Request::Install{ "doxygen"_ms },
Request::Install{ "catch2"_ms },
Request::Install{ "cmake"_ms },
Request::Install{ "cxx-compiler"_ms },
Request::Install{ "menuinst"_ms },
Request::Install{ "pywin32"_ms },
Request::Install{ "sphinx-book-theme"_ms },
Request::Install{ "sphinx-copybutton"_ms },
Request::Install{ "pre-commit"_ms },
Request::Install{ "mitmproxy"_ms },
},
}
);
}

/**
* Create a conflict due to a pin.
*/
Expand Down Expand Up @@ -641,6 +681,7 @@ TEST_CASE("Create problem graph", "[mamba::solver]")
std::pair{ "PubGrub example", &create_pubgrub },
std::pair{ "Harder PubGrub example", &create_pubgrub_hard },
std::pair{ "PubGrub example with missing packages", &create_pubgrub_missing },
std::pair{ "Many missing top-level packages", &create_many_missing_top_level },
std::pair{ "Pin conflict", &create_pin_conflict },
std::pair{ "PyTorch CPU", &create_pytorch_cpu },
std::pair{ "PyTorch Cuda", &create_pytorch_cuda },
Expand Down
Loading