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.

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

Add JGF reader support #521

Merged
merged 11 commits into from Oct 9, 2019
Merged

Add JGF reader support #521

merged 11 commits into from Oct 9, 2019

Conversation

@dongahn
Copy link
Contributor

dongahn commented Sep 29, 2019

This PR adds JGF reader support. This should not be merged before PR #520.

  • Introduce reader classes into the new resource/readers directory and refactor resource_graph_db_t into its own directory (resource/store). Reader classes not only refactor the existing reader code (i.e., GRUG and Hwloc) but also add the new JGF reader code.

  • Integrate the new reader support into resource-query and resource module.

  • As part of that, change the command line options for resource-query and the load options for resource module. For resource-query, --grug and --hwloc-xml are deprecated and --load-file and --load-format are added instead. --hwloc-whitelist is now changed to --load-whitelist. For the resource module, grug-conf, hwloc-xml, and hwloc-whitelist have been deprecated. Instead, load-file, load-format, and load-whitelist are used.

  • Adjust the existing tests for these option name changes.

  • Add three new test cases to ensure the correctness of the JGF reader.

@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Sep 30, 2019

t3014-resource-basic-jgf.t tests are failing in the Travis CI. Let me look.

@dongahn dongahn force-pushed the dongahn:readers branch from eb8386a to aa7f238 Sep 30, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 30, 2019

Codecov Report

Merging #521 into master will decrease coverage by 0.34%.
The diff coverage is 79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #521      +/-   ##
==========================================
- Coverage   76.38%   76.03%   -0.35%     
==========================================
  Files          59       69      +10     
  Lines        6221     6481     +260     
==========================================
+ Hits         4752     4928     +176     
- Misses       1469     1553      +84
Impacted Files Coverage Δ
resource/utilities/command.hpp 100% <ø> (ø) ⬆️
resource/schema/resource_graph.hpp 59.25% <ø> (-2.81%) ⬇️
resource/readers/resource_spec_grug.hpp 100% <ø> (ø)
resource/store/resource_graph_store.cpp 100% <100%> (ø)
resource/readers/resource_reader_grug.hpp 100% <100%> (ø)
resource/readers/resource_reader_jgf.hpp 100% <100%> (ø)
resource/utilities/command.cpp 68.75% <100%> (ø) ⬆️
resource/readers/resource_reader_base.hpp 100% <100%> (ø)
resource/store/resource_graph_store.hpp 100% <100%> (ø)
resource/readers/resource_reader_hwloc.hpp 100% <100%> (ø)
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adea512...b86a85f. Read the comment docs.

@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Oct 1, 2019

Travis is green now. @SteVwonder, @tpatki and @milroy: this PR should also be ready for your reviews. This should just be merged after PR #520, though.

@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Oct 1, 2019

PR #523 has been merged. This needs to be rebased to the upstream master.

@dongahn dongahn force-pushed the dongahn:readers branch from aa7f238 to 59dee20 Oct 1, 2019
@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Oct 1, 2019

Rebased to the current master.

@dongahn dongahn force-pushed the dongahn:readers branch 2 times, most recently from 1f1f403 to f1d8b07 Oct 2, 2019
@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Oct 2, 2019

Just rebased to the master with PR #520 and forced a push.

string load_file; /* load file name */
string load_format; /* load reader format */
string load_whitelist; /* load resource whitelist */
Comment on lines +56 to +58

This comment has been minimized.

Copy link
@tpatki

tpatki Oct 4, 2019

Member

@dongahn:
Shall provide a detailed review soon, but had a quick comment.

Can we change the variable names to reflect that this is for a graph? Maybe something like graph_input_file, graph_format is better than load_file and load_format?

Also, the whitelist is currently specific to hwloc. Do we expect any other future graph formats (beyond the three we have) to use the whitelist?

If not, I'm wondering if set_whitelist should be specific to the hwloc reader class as opposed to the base class. The current design is fine, I'm suggesting this to make the code easily readable, less verbose for the two readers that don't use it, and having a clearer check specific to hwloc reader here instead:

if (rd->set_whitelist (ctx->args.load_whitelist) < 0)

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

The variable names are matched with the command line options. There other graph related option in resource query so I didn't use graph. If you feel strongly about these, I will be happy to change them, though. Let me know.

Yes there is a good chance that white list can be supported by other readers and I made this a method in the base class for future extension.

if ( (rd = create_resource_reader (ctx->args.load_format)) == nullptr) {
flux_log (ctx->h, LOG_ERR, "creating load reader");
Comment on lines 442 to 443

This comment has been minimized.

Copy link
@tpatki

tpatki Oct 4, 2019

Member

Space between parentheses.

Shouldn't the error message be "Can't create load reader"?

Also, a clear set of parentheses around the create_resource_reader (ctx->args.load_format)) == nullptr before assigning to rd will make the if statement more readable. Readability of if (return_val = function () == nullptr) format is confusing.

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

If you are talking about the extra whitespace in if ( (rd =, this is a pretty common practive. In fact the famous Steven book on "Unix system programming" recommends this. If this was defined in our style guide, I can change it. But I don't recall if I have seen this rewuirement.

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

Shouldn't the error message be "Can't create load reader"?

This was intentional as LOG_ERR generates "err" in the message prefix. But I can see why your message makes more sense. So I will change it. Thanks.

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

Also, a clear set of parentheses around the create_resource_reader (ctx->args.load_format)) == nullptr before assigning to rd will make the if statement more readable. Readability of if (return_val = function () == nullptr) format is confusing.

Hmmm yes that's why I had the extra parenthesis before rc and the end of the method call. Maybe I am missing something.

}
o = get_string_blocking (h, k);
hwloc_xml = json_string_value (o);
if ( (rc = ctx->db.load (hwloc_xml, rd, v, rank)) < 0) {

This comment has been minimized.

Copy link
@tpatki

tpatki Oct 4, 2019

Member

Space between parentheses.

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

See above.

}
buffer << in_file.rdbuf ();
in_file.close ();
if ( (rc = ctx->db.load (buffer.str (), rd)) < 0) {

This comment has been minimized.

Copy link
@tpatki

tpatki Oct 4, 2019

Member

Not sure if this is part of the style guide, but there's an extra space between the parenthesis here.

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

Please see above.

}
o = get_string_blocking (h, k);
hwloc_xml = json_string_value (o);
if ( (rc = ctx->db.load (hwloc_xml, rd, rank)) < 0) {

This comment has been minimized.

Copy link
@tpatki

tpatki Oct 4, 2019

Member

Space between parentheses.

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

Same.

if (ctx->args.load_whitelist != "") {
if (rd->set_whitelist (ctx->args.load_whitelist) < 0)
flux_log (ctx->h, LOG_ERR, "setting whitelist");
if (!rd->is_whitelist_supported ())
flux_log (ctx->h, LOG_WARNING, "whitelist unsupported");
Comment on lines +446 to +450

This comment has been minimized.

Copy link
@tpatki

tpatki Oct 4, 2019

Member

Not sure if whitelisting should be a part of the base class (see earlier comment on whether we expect future graph formats other than hwloc to have whitelists).

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

Yes exactly. This is there in the base class for future extension.

rc = -1;
flux_log (ctx->h, LOG_ERR, "error in generating resources");
if (populate_resource_db_kvs (ctx, rd) < 0) {
flux_log (ctx->h, LOG_ERR, "loading resources from the KVS");

This comment has been minimized.

Copy link
@tpatki

tpatki Oct 4, 2019

Member

Maybe change the message to "Error loading resources from KVS"?

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

My understanding was "err" will be emitted with LOG_ERR and this was part of my effort to avoid redundancy. Let me double check though.

int get_rank ();

private:
vtx_t emit_vertex (ggv_t u, gge_t e, const gg_t &recipe,

This comment has been minimized.

Copy link
@tpatki

tpatki Oct 4, 2019

Member

Change ggv_t to vtx_t?

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

Actually here v and e are an vertex and edge of the resource generation graph. So ggv_t should be correct I think.

~dfs_emitter_t ();

void tree_edge (gge_t e, const gg_t &recipe);
void finish_vertex (ggv_t u, const gg_t &recipe);

This comment has been minimized.

Copy link
@tpatki

tpatki Oct 4, 2019

Member

Change ggv_t to vtx_t?

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

Same as above.

int gen_id (gge_t e, const gg_t &recipe,
int i, int sz, int j);

map<ggv_t, vector<vtx_t>> m_gen_src_vtx;

This comment has been minimized.

Copy link
@tpatki

tpatki Oct 4, 2019

Member

Change ggv_t to vtx_t?

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

Same.

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,

This comment has been minimized.

Copy link
@tpatki

tpatki Oct 4, 2019

Member

ggv_t to vtx_t

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

Ditto.

int id = 0;

if (src_v == boost::graph_traits<resource_graph_t>::null_vertex ()) {
// ROOT!!

This comment has been minimized.

Copy link
@tpatki

tpatki Oct 4, 2019

Member

Minor: maybe change the comment to be // Root vertex of graph. as opposed to ROOT!!

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

Sure. I will change it.

vtx_t src_vtx, tgt_vtx;
ggv_t src_ggv = source (e, recipe);
ggv_t tgt_ggv = target (e, recipe);
Comment on lines +285 to +288

This comment has been minimized.

Copy link
@tpatki

tpatki Oct 4, 2019

Member

I'm unsure about legitimate use of ggv_t versus vtx_t. Will revisit these once I understand this more clearly.

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

I left a comment to an issue ticket where you asked this question. Thanks

ctx->params.grug = "conf/default";

gettimeofday (&st, NULL);
if ( (rd = create_resource_reader (ctx->params.load_format)) == nullptr) {

This comment has been minimized.

Copy link
@tpatki

tpatki Oct 4, 2019

Member

Space between parenthesis

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

Same as above.


gettimeofday (&st, NULL);
if ( (rc = ctx->db.load (buffer.str (), rd)) != 0) {

This comment has been minimized.

Copy link
@tpatki

tpatki Oct 4, 2019

Member

Space between parenthesis.

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

Same.

@tpatki

This comment has been minimized.

Copy link
Member

tpatki commented Oct 4, 2019

@dongahn: I made a few very minor comments, I will have to look at this in detail (esp the new JGF reader and tests) and I shall get to it soon.

@tpatki

This comment has been minimized.

Copy link
Member

tpatki commented Oct 4, 2019

Also, this will need to be rebased. t3014 and t4007 were part of the variation-aware scheduler that was just merged, and those test cases will have to be renamed to a different number.

@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Oct 4, 2019

Thanks @tpatki. I left my responses above. I will change a few things you spotted and as part of this I will adjust the test numbering that conflicts with your variation PR.

Refactor resource_graph_db_t into its own directory
(resource/store). This will replace the same named
class in resource/schema/resource_graph.hpp when
integrated into the rest of the system.

Add two load() methods into this class which
takes in an object of the base reader class
to decode the input string into its resource graph.
How it is decoded is delegated down to the reader
object whose runtime type must be a derived
class of the base reader class.

Introduce resource_graph_metadata_t that contains
metadata about the resource graph. This helps simplify
reader class methods.

Add readers/resource_reader_base.[hpp|cpp]
which define the base reader interfaces. They
include a few pure virtual abstract interfaces
that upcoming derived reader classes must override.
The main methods of this class are unpack() and unpack_at().

The unpack() method deserializes the given string into
a resource graph.

The unpack_at() method unpacks the given string into
a resource graph and then grafts the top-level vertices
to an existing vertex in the resource graph.
@dongahn dongahn force-pushed the dongahn:readers branch from f1d8b07 to 0fbc391 Oct 4, 2019
@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Oct 4, 2019

I just forced a push. I rebased this to the latest master with @tpatki's variation aware scheduling work. And I believe I have also addressed all the issues identified from the review discussions thus far.

@SteVwonder and/or @milroy : do you have other reviews?

@milroy

This comment has been minimized.

Copy link
Member

milroy commented Oct 4, 2019

@dongahn I am reviewing this PR now and will be finished with it today.

@milroy
milroy approved these changes Oct 4, 2019
Copy link
Member

milroy left a comment

I added a few questions and nits regarding parentheses, but this PR looks good to me.

std::map<subsystem_t, vtx_t> roots;
std::map<std::string, std::vector <vtx_t>> by_type;
std::map<std::string, std::vector <vtx_t>> by_name;
std::map<std::string, std::vector <vtx_t>> by_path;

This comment has been minimized.

Copy link
@milroy

milroy Oct 4, 2019

Member

by_path eventually should be changed to std::map<std::string, vtx_t> or the like (see issue #518). That's beyond the scope of this PR, but I'm noting it here for future reference. I'll try to address it in my upcoming PR based on issue #511.

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

Agreed! The changes like this are now lumped into either hardening and/or optimization project in my kanban board.

@@ -0,0 +1,465 @@
/*****************************************************************************\
* Copyright (c) 2014 Lawrence Livermore National Security, LLC. Produced at

This comment has been minimized.

Copy link
@milroy

milroy Oct 4, 2019

Member

I think this a new file, so the copyright should be 2019. That said, much of the content is from an older file, so perhaps the 2014 date is correct here.

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

I don't know what is the conversion when changes are made across multiple years. Like 2014 - 2019. I will change it that way and see how it looks.

for ( ; ei != ee; ++ei) {
if (target (*ei, g) == tgt_v) {
e = (*ei);
return e;
}
Comment on lines +152 to +157

This comment has been minimized.

Copy link
@milroy

milroy Oct 4, 2019

Member

Does this loop exist to avoid calling boost::edge (src_v, tgt_v, g) and its O(E/V) complexity for vecS edges?

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

Yes, once you find the matching edge, you don't need to further traverse out edges so you return.

As we discussed before, being able to find an edge with O(map) like performance will greatly be helpful.

This comment has been minimized.

Copy link
@milroy

milroy Oct 4, 2019

Member

Yes, that makes sense. I've higher performance lookups in my WIP on issue #511.

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

Great! You may want to split your WIP into multiple PRs and get some early reviews before it gets too large. It seems a faster edge lookup as a standalone topic can make a good enough PR. Also there are some performance benchmarks you can try your lookup logic to see how much it improves the performance.

for (tgt_it = m.by_type[recipe[tgt_ggv].type].begin();
tgt_it != m.by_type[recipe[tgt_ggv].type].end(); tgt_it++) {
Comment on lines 347 to 348

This comment has been minimized.

Copy link
@milroy

milroy Oct 4, 2019

Member

Nit: .begin() and .end() need spaces after the parentheses.

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

Good catch! I will fix them.

for (tgt_it = m.by_type[recipe[tgt_ggv].type].begin();
tgt_it != m.by_type[recipe[tgt_ggv].type].end(); tgt_it++) {
Comment on lines 327 to 328

This comment has been minimized.

Copy link
@milroy

milroy Oct 4, 2019

Member

Nit: .begin() and .end() need spaces after the parentheses.

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

Ditto!

return (gen_meth_t)str2genmeth[i].e;
return (gen_meth_t)str2genmeth[i].e;
Comment on lines 143 to 144

This comment has been minimized.

Copy link
@milroy

milroy Oct 4, 2019

Member

Should these casts have spaces after the close parentheses?

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

I don't think we have that required by our coding guide. My preference is not to have a space for casting.

vtx_basename_map_t v_bn_map = get(&resource_pool_gen_t::basename, g);
vtx_size_map_t v_sz_map = get(&resource_pool_gen_t::size, g);
//vtx_unit_map_t v_ut_map = get(&resource_pool_gen_t::unit, g);
vtx_subsystem_map_t v_ss_map = get(&resource_pool_gen_t::subsystem, g);
edg_relation_map_t e_rel_map = get(&relation_gen_t::relation, g);
edg_gen_method_map_t e_gen_map = get(&relation_gen_t::gen_method, g);
edg_multi_scale_map_t e_ms_map = get(&relation_gen_t::multi_scale, g);
Comment on lines 191 to 197

This comment has been minimized.

Copy link
@milroy

milroy Oct 4, 2019

Member

Change to get (...?

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

Good catch. I will fix those.

if (strncmp(obj->name, "cuda", 4) == 0)
id = atoi (obj->name + 4);
else if (strncmp(obj->name, "opencl", 6) == 0) {
Comment on lines 234 to 236

This comment has been minimized.

Copy link
@milroy

milroy Oct 4, 2019

Member

Nits: spaces after strncmp.

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

Will do.

for (auto kv : g[v].paths) {
g[v].idata.member_of[kv.first] = "*";
m.by_path[kv.second].push_back (v);
if (std::count(kv.second.begin(), kv.second.end(), '/') == 1)

This comment has been minimized.

Copy link
@milroy

milroy Oct 4, 2019

Member

Spaces after begin and end.

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 4, 2019

Author Contributor

Thanks. (Maybe we need some format enforcer :-).

@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Oct 4, 2019

Thanks @milroy! I will address the issues above and push my changes.

/*! Unpack str into a resource graph and graft
* the top-level vertices to vtx.
*
* \param g resource graph
* \param m resource graph meta data
* \param vtx parent vtx at which to graft the deserialized graph
* \param str resource set string
* \param rank assign rank to all of the newly created resource vertices
* \return 0 on success; non-zero integer on an error
*/
virtual int unpack_at (resource_graph_t &g, resource_graph_metadata_t &m,
vtx_t &vtx, const std::string &str, int rank = -1) = 0;
Comment on lines +53 to +64

This comment has been minimized.

Copy link
@milroy

milroy Oct 7, 2019

Member

param vtx will have edges to and/or from each of the subsystem roots of the unpacked resource graph, correct? Also, is vtx in the original graph but not in the unpacked graph?

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 7, 2019

Author Contributor

@milroy: great question. Easy one first:

Also, is vtx in the original graph but not in the unpacked graph?

Yes, it is a vertex that should only be in the original graph. This interface is currently only wired with the hwloc reader to attach a new set of subgraph created by each of the ranked hwloc xml objects into the root vertex: the cluster vertex.

param vtx will have edges to and/or from each of the subsystem roots of the unpacked resource graph, correct?

The intent is broader than this.

Say, the unpacked graph is node10->core0. While node10 is the root of the subgraph, it is not the root of the overall graph. I detect the root of the subgraph by parsing each vertex's paths field. If node10 vertex's paths has containment: /cluster/node10, it won't be identified as the root of the containment subsystem.

Also if the unpacked graph is node10->core0, node20->core30, I expect that this interface attach both node10 and node20 to the vtx. (We don't currently have this use case as hwloc reader reads one rank at a time.) If this were to work, it is almost like the caller should specify what those vertices are, which need to be attached to vtx. ..

If you want to propose something more general than this interface, which can help your work, maybe we can work on an interface revision after this PR lands?

This comment has been minimized.

Copy link
@milroy

milroy Oct 7, 2019

Member

This interface is currently only wired with the hwloc reader to attach a new set of subgraph created by each of the ranked hwloc xml objects into the root vertex: the cluster vertex.

But the intention is that eventually vtx can be any vertex in the original graph?

While node10 is the root of the subgraph, it is not the root of the overall graph. I detect the root of the subgraph by parsing each vertex's paths field.

That's sensible. I was abusing the term "subsystem roots" as referring to the top-level node(s) in the unpacked graph (the subgraph). Can the subgraph have multiple roots (i.e. the subgraph contains vertices in multiple subsystems)?

Also if the unpacked graph is node10->core0, node20->core30, I expect that this interface attach both node10 and node20 to the vtx. (We don't currently have this use case as hwloc reader reads one rank at a time.) If this were to work, it is almost like the caller should specify what those vertices are, which need to be attached to vtx. ..

This is my main curiosity. The path to vertex mapping is many-to-one since each vertex can have a path per subsystem: g[v].paths[subsys] = prefix + "/" + g[v].name;. This suggests to me that node10 could be attached to one vtx from each subsys where g[node10].paths[subsys] is defined.

If you want to propose something more general than this interface, which can help your work, maybe we can work on an interface revision after this PR lands?

Yes, I think we will need a more general interface which accepts a node to vertex map. I agree, that work is out of scope for this PR.

This comment has been minimized.

Copy link
@SteVwonder

SteVwonder Oct 9, 2019

Member

Yes, I think we will need a more general interface which accepts a node to vertex map. I agree, that work is out of scope for this PR.

Do you want to track the idea with a new issue?

@milroy

This comment has been minimized.

Copy link
Member

milroy commented Oct 8, 2019

@dongahn it looks like you still need to add spaces after a few function calls. After that I can merge this PR, per our discussion with @SteVwonder.

@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Oct 8, 2019

@dongahn it looks like you still need to add spaces after a few function calls. After that I can merge this PR, per our discussion with @SteVwonder.

Yes, I planned to do this after some urgent tasks. Stay tuned.

@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Oct 9, 2019

@milroy: OK, I pushed all the requested changes. Once you give a quickly review on those intermediate commits, I will squash them before this gets merged. Let me know.

@milroy

This comment has been minimized.

Copy link
Member

milroy commented Oct 9, 2019

@dongahn looks good! The copyright year range is a good solution.

dongahn added 10 commits Sep 29, 2019
Introduce resource_reader_grug_t by deriving it from
resource_reader_base_t.

Refactor GRUG reader code into resource_reader_grug_t
and implement the unpack() method.

Adjust this code to wean off of the old
resource_graph_db_t and make use of resource_graph_t
and resource_graph_metadata_t instead.

The unpack_at() method is unsupported. Resource
whitelist is not supported either, and this is indicated
by returning false from is_whitelist_supported() method.
Introduce resource_reader_hwloc_t by deriving it from
resource_reader_base_t.

Refactor Hwloc reader code into resource_reader_hwloc_t
and implement the unpack() and unpack_at() methods.

Adjust this code to wean off of the old
resource_graph_db_t and make use of resource_graph_t
and resource_graph_metadata_t instead.

Add a bit better error message support for hwloc
API calls from the original hwloc reader code.
Introduce resource_reader_jgf_t by deriving it from
resource_reader_base_t.

First, fetch each JGF node from the "nodes" key
from the JGF string, and add a vertex of the
resource graph data while building vmap that keeps
track of the JGF nodeid-vertex pair using that info.

Next, fetch each JGF node from the "edges" key
and add an edge of the resource graph data using
that info in combination with the source and
target vertices provided by the vmap.

As part of resource graph vertex and edge population,
we fill in graph infrastructure information
such as idata.member_of as well as resource graph
metadata (e.g., roots of each detected subsystem).

Detect each unique subsystem and its root vertex
by using the paths field of each JGF node.
If the paths field of a JGF node contains a path with
one slash (e.g., "containment": "/cluster0"),
deem it as the root of that subsystem.

Implement unpack() method while leaving unpack_at()
method unsupported. Hence, is_whitelist_supported()
returns false.
Instantiate an object of a derived resource reader class
that corresponds to the input string (reader type name)
and return it as the object of the resource reader base
class.

Allow the client code to be written as the base class,
making it future proof.

Use shared_ptr class to enhance safety.
Integrate the new resource reader scheme into the entire
resource infastructure and the existing tests.

- Drop the old reader/generator code in generators/gen.[hpp|cpp].

- Delete the resource_graph_db_t class from
schema/resource_graph.hpp as the new definitions in
store/resource_graph_store.[hpp|cpp] will be wired in.

- Update resource/Makefile.am with respect to the above removals
and the new resource reader files that were previously added.
Allow the new code to make into the resource convenient library.
Add jansson as an extra dependency as required
by the JGF reader as well.

- Change the command line options for resource-query and
the load options for resource match module.
For resource-query, --grug and --hwloc-xml are deprecated and
--load-file and --load-format are added instead.
--hwloc-whitelist is now changed to --load-whitelist.
For the resource module, grug-conf, hwloc-xml,
and hwloc-whitelist have been deprecated.
Instead, load-file, load-format, and load-whitelist are
used.

- Adjust resource-query and resource module code with respect
to the new resource_graph_db_t and resource_graph_metadata_t
definitions.

- populate_resource_db in resource-query and resource module
code now makes use of the new resource reader support.

- Update all of the sharness test cases to use the new
cli and module-load options.
Add 3 machine definitions written in JGF in
data/resource/jgfs (in parallel to data/resource/grugs).

Will be used by future JGF reader sharness tests.

hwloc_4core.json contains a very small machine configuration
with a total of 1 compute node with 4 compute cores.

power.json describes the same system specified by
data/resource/grugs/power.graphml.

tiny.json describes the same system specified by
data/resource/grugs/tiny.graphml

When power.json or tiny.json is loaded into
resource-query and resource module, these programs
must behave exactly same as they were loaded with
the corresponding GRUG files.

In fact, future test codes will be written based
ont this property.
Copied from t3001-resource-basic.t.
Only change the machine specification from GRUG to JGF.

Because we use the exactly same resource-query commands,
the test outputs must match with the results of
t3001-resource-basic.t.
Copied from t3010-resource-power.t.
Only change the machine specification from GRUG to JGF.

Because we use the exactly same resource-query commands,
the test outputs must match with that of
t3010-resource-power.t.

Adjust this test code to ensure the JGF reader
works correctness when there are multiple
subsystems in the JGF spec (i.e. containment and power).
Add simple tests to ensure the JGF reader works when
used within the resource module as well.
@dongahn dongahn force-pushed the dongahn:readers branch from e6bee55 to b86a85f Oct 9, 2019
@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Oct 9, 2019

@milroy: I squashed all the intermediate commits. Once Travis turns green, please go ahead and merge. Thank you @milroy, @tpatki and @SteVwonder for the review.

@milroy milroy merged commit 16e72c3 into flux-framework:master Oct 9, 2019
3 checks passed
3 checks passed
codecov/patch 79% of diff hit (target 76.38%)
Details
codecov/project Absolute coverage decreased by -0.34% but relative coverage increased by +2.62% compared to adea512
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.