Skip to content
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

Jump to bottom

graph: use builtin bidirectional graph #1240

Merged
merged 3 commits into from
Jul 11, 2024
Merged

graph: use builtin bidirectional graph #1240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6575bee
graph: use builtin bidirectional graph
trws Jul 7, 2024
9fced57
jgf/test: remove "in" edges from stored jgf files
trws Jul 8, 2024
742812d
jgf/reader: optimize get_parent_vtx
trws Jul 8, 2024
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
5 changes: 0 additions & 5 deletions resource/readers/resource_reader_grug.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,11 +188,6 @@ void dfs_emitter_t::emit_edges (gge_t ge, const gg_t &recipe,
g[e].idata.member_of[recipe[ge].e_subsystem]
= recipe[ge].relation;
g[e].name[recipe[ge].e_subsystem] = recipe[ge].relation;
if ( (rc = raw_edge (tgt_v, src_v, e)) < 0)
return;
g[e].idata.member_of[recipe[ge].e_subsystem]
= recipe[ge].rrelation;
g[e].name[recipe[ge].e_subsystem] = recipe[ge].rrelation;
}

vtx_t dfs_emitter_t::emit_vertex (ggv_t u, gge_t e, const gg_t &recipe,
Expand Down
12 changes: 0 additions & 12 deletions resource/readers/resource_reader_hwloc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,6 @@ int resource_reader_hwloc_t::walk_hwloc (resource_graph_t &g,
name, properties, size, rank);
valid_ancestor = v;
std::string relation = "contains";
std::string rev_relation = "in";
edg_t e;
bool inserted; // set to false when we try and insert a parallel edge

Expand All @@ -378,17 +377,6 @@ int resource_reader_hwloc_t::walk_hwloc (resource_graph_t &g,
if (add_metadata (m, e, parent, v, g) < 0)
return -1;

tie (e, inserted) = add_edge (v, parent, g);
if (!inserted) {
errno = ENOMEM;
m_err_msg += "error inserting a new edge: "
+ g[v].name + " -> " + g[parent].name + "; ";
return -1;
}
g[e].idata.member_of[subsys] = rev_relation;
g[e].name[subsys] = rev_relation;
if (add_metadata (m, e, v, parent, g) < 0)
return -1;
}

hwloc_obj_t curr_child = NULL;
Expand Down
35 changes: 16 additions & 19 deletions resource/readers/resource_reader_jgf.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1068,10 +1068,12 @@ int resource_reader_jgf_t::unpack_edges (resource_graph_t &g,
goto done;
}
json_object_foreach (name, key, value) {
g[e].name[std::string (key)]
= std::string (json_string_value (value));
g[e].idata.member_of[std::string (key)]
= std::string (json_string_value (value));
auto skey = std::string (key);
auto sval = std::string (json_string_value (value), json_string_length (value));
if (sval == std::string ("in"))
continue;
g[e].name[skey] = sval;
g[e].idata.member_of[skey] = sval;
}
// add this edge to by_outedges metadata
auto iter = m.by_outedges.find (vmap[source].v);
Expand Down Expand Up @@ -1224,27 +1226,22 @@ int resource_reader_jgf_t::get_subgraph_vertices (resource_graph_t &g,
int resource_reader_jgf_t::get_parent_vtx (resource_graph_t &g,
vtx_t vtx,
vtx_t &parent_vtx)

{
vtx_t next_vtx;
boost::graph_traits<resource_graph_t>::out_edge_iterator ei, ei_end;
boost::tie (ei, ei_end) = boost::out_edges (vtx, g);
boost::graph_traits<resource_graph_t>::in_edge_iterator ei, ei_end;
boost::tie (ei, ei_end) = boost::in_edges (vtx, g);
int rc = -1;

for (; ei != ei_end; ++ei) {
next_vtx = boost::target (*ei, g);
for (const auto &paths_it : g[vtx].paths) {
// check that the parent's name exists in the child's path before the child's name
if (paths_it.second.find (g[next_vtx].name) != std::string::npos &&
paths_it.second.find (g[vtx].name) > paths_it.second.find (g[next_vtx].name)) {
parent_vtx = next_vtx;
rc = 0;
break;
}
}
}
next_vtx = boost::source (*ei, g);
if (g[*ei].name.contains ("containment")) {
parent_vtx = next_vtx;
rc = 0;
break;
Comment on lines +1237 to +1240
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good for containment, but I seem to remember there was a corner case that motivated @zekemorton to implement the check this way. Zeke, can you comment here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, when the graph was a directedS graph, the approach I'm using here would not have worked at all since there wouldn't be a way to get in_edges like this, and even if we iterated all vertices to get them there would have been in_edges from the children as well. As long as all graphs follow the invariant that there's always a containment tree, and it's always single parent, this should be fine. If we have a case where one of those is violated, it would be wrong, but I'd want to know what that is and why it's invalidating the invariant.

This reminds me by the way, you asked why it is that everything has to be in the containment hierarchy, every component I mean, and this is one example of why. If we want there to be other non-containment hierarchies (things like networking can't be hierarchies, but they shouldn't be anyway, they should be extra links between things that exist in other hierarchies) we could do that, but it would mean having the hierarchy the thing is resident in be part of the vertex, or the vertex type perhaps, and then having some operations that need to traverse more than one.

Copy link
Member

@milroy milroy Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want there to be other non-containment hierarchies (things like networking can't be hierarchies, but they shouldn't be anyway, they should be extra links between things that exist in other hierarchies)

This comes up in the power JGF, where the power subsystem is also a hierarchy. The current implementation can return the parent in the power subsystem instead of the parent in the containment hierarchy, which actually might not be desirable.

I think returning the parent vertex in the containment hierarchy will work well, and it's lower complexity.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make it capable of doing both with the same complexity if we pass in which edge type to consider, or have it embedded in the vertex type. Maybe something to consider for later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation...

In case it wasn't clear (I think it was), I meant current flux-sched master, not the change in the PR.

}
}

return rc;
return rc;
}


Expand Down
10 changes: 0 additions & 10 deletions resource/readers/resource_reader_rv1exec.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,16 +189,6 @@ int resource_reader_rv1exec_t::add_edges (resource_graph_t &g,
if (add_metadata (g, m, e, src, dst) < 0)
goto error;

tie (e, inserted) = add_edge (dst, src, g);
if (!inserted) {
errno = ENOMEM;
goto error;
}
g[e].idata.member_of[subsys] = rev_relation;
g[e].name[subsys] = rev_relation;
if (add_metadata (g, m, e, dst, src) < 0)
goto error;

return 0;

error:
Expand Down
4 changes: 1 addition & 3 deletions resource/schema/resource_graph.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,11 @@
#define RESOURCE_GRAPH_HPP

#include "resource/schema/resource_data.hpp"
#include <boost/config.hpp>
#include <boost/graph/adjacency_list.hpp>
#include <boost/graph/graphml.hpp>
#include <boost/graph/graphviz.hpp>

#include <utility>
#include <memory>

namespace Flux {
namespace resource_model {
Expand All @@ -31,7 +29,7 @@ using rname_t = std::string resource_relation_t::*;
using rinfra_t = relation_infra_t resource_relation_t::*;
using resource_graph_t = boost::adjacency_list<boost::vecS,
boost::vecS,
boost::directedS,
boost::bidirectionalS,
resource_pool_t,
resource_relation_t,
std::string>;
Expand Down
Loading
Loading