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

Tidy up JGF match writer support #520

Merged
merged 7 commits into from Oct 2, 2019
Merged

Conversation

@dongahn
Copy link
Contributor

dongahn commented Sep 29, 2019

This PR cleans up match writer support in preparation for the upcoming JGF reader support. It contains:

  • Drop support for pretty printing for JSON-based writers
  • Adjust the schema and internals of JGF format to support general cases and the future JGF reader
  • Two bug fixes
@dongahn dongahn requested review from SteVwonder and milroy Sep 29, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 29, 2019

Codecov Report

Merging #520 into master will decrease coverage by 0.31%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #520      +/-   ##
==========================================
- Coverage   76.55%   76.23%   -0.32%     
==========================================
  Files          58       58              
  Lines        6291     6165     -126     
==========================================
- Hits         4816     4700     -116     
+ Misses       1475     1465      -10
Impacted Files Coverage Δ
resource/writers/match_writers.hpp 100% <ø> (ø) ⬆️
resource/utilities/resource-query.cpp 49.71% <ø> (ø) ⬆️
resource/writers/match_writers.cpp 95.28% <100%> (+1.27%) ⬆️
resource/generators/gen.cpp 86.96% <100%> (-0.08%) ⬇️

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 2aab88c...03f95cc. Read the comment docs.

@dongahn dongahn mentioned this pull request Sep 29, 2019
@dongahn dongahn requested a review from tpatki Sep 30, 2019
@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Sep 30, 2019

@SteVwonder, @tpatki and @milroy: this PR is ready to be reviewed. This should be merged before PR #521, which actually has the JGF reader support.

@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:tidy-writers branch from 2bd1497 to 7eaf95c Oct 1, 2019
@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Oct 1, 2019

Rebased to the current master.

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

milroy left a comment

I've indicated a few places where spaces need to be inserted for adherence to the style guide, but apart from those minor nitpicks this PR looks good to me.

I don't think the spaces must be added to merge this PR; that's up to you @dongahn.

@@ -411,14 +411,14 @@ int resource_generator_t::check_hwloc_version (string &m_err_msg) {
return 0;
}

ggv_t resource_generator_t::add_new_vertex(resource_graph_db_t &db,
const ggv_t &parent,
vtx_t resource_generator_t::add_new_vertex(resource_graph_db_t &db,

This comment has been minimized.

Copy link
@milroy

milroy Oct 1, 2019

Member

Nit: based on the flux style guide, there should be a space between "vertex" and "(" in resource_generator_t::add_new_vertex(.

if (!supported_resource) {
valid_ancestor = parent;
} else {
const string subsys = "containment";
ggv_t v = add_new_vertex(db, parent, id,
vtx_t v = add_new_vertex(db, parent, id,

This comment has been minimized.

Copy link
@milroy

milroy Oct 1, 2019

Member

There should be a space after "vertex" here as well.

const std::string &err_message () const;

private:
int check_hwloc_version (std::string &m_err_msg);
ggv_t add_new_vertex(resource_graph_db_t &db, const ggv_t &parent, int id,
vtx_t add_new_vertex(resource_graph_db_t &db, const vtx_t &parent, int id,

This comment has been minimized.

Copy link
@milroy

milroy Oct 1, 2019

Member

Needs a space after "vertex."

@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Oct 1, 2019

Great! Thanks you @milroy. I will go ahead and change the spacing issues since I will have to rebase this anyway. Once that's done, I would say we merge this so that @tpatki and/or @SteVwonder can focus on PR #521, which would be more involved.

dongahn added 7 commits Sep 20, 2019
Change the use of ggv_t to vtx_t. ggv_t is the data type of
a vertex of a graph generation recipe graph whereas vxt_t
is the data type of a vertex of the actual resource graph.

Found a few sites where ggv_t type is used when vtx_t
should have been used during a code review and fix them.
Problem: It is becoming difficult to maintain two difference
versions (pretty vs. unpretty) of writers of the same type.
Further the value of the pretty versions is marginal as the
output from a unpretty writer can easily be made
human readable by piping to the jq command.

Drop the pretty writer support for JSON-based writers.

Adjust testing coverage accordingly.

Will consider to add back this support when we convert
our JSON writers to use either the Jansson library
or its C++ equivalent.

If and when this is revived, the implementation should be added
such that pretty writers will be supported without
having to introduce their own classses that contain
duplicate codes with unpretty versions.
Correctly detect the unmatch case to write nothing.

Wasn't taking into account a new-line character
in detecting unmatch cases, and as a result these
writers were writing header information (e.g.,
version key and execution key) with no match data!
Problem: the old scheme uses the resource graph's
vertex index to denote the unique id for
each JGF vertex. As the resource graph's vertex
index is the same as uniq_id at the top
level Flux instance, this has not been a problem.
It is "generated" from the graph's vertex index.
However, this will become a problem to support
general cases. For example, at a nested Flux instance,
uniq_id will be "given" by the JGF object
from its parent, the resource graph's vertex index
and uniq_id will differ.
Similarly, if an JGF object is constructed
either manually or by Kubernetes, the same
problem will occur.

Use uniq_id as the JGF node id to generalize
our JGF writer support.
Problem: The paths field was previously omited
with a hope that the paths can be reconstructed
by the future JGF reader.
But it appears that this would require lots of code
at the reader side and would be inefficient regardless.

Modify the JGF writer code to emit the paths of each
graph vertex within each subsystem.
(Doing this in the writer side is simpler
at the expense of using more memory/storage).

Will consider JGF schema compression as part of
future optimization phase.
Problem: The old scheme is to concatenate the edge's
relationship information with respect to each subsystem
using some delimiters. While this is condense, this
can lead to unnecessary complexity in the future
JGF reader code.

Emit the relationship information for each subsystem
that the edge represents as a unique sub-key within
the name key.
@dongahn dongahn force-pushed the dongahn:tidy-writers branch from 7eaf95c to 03f95cc Oct 2, 2019
@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Oct 2, 2019

@milroy" OK. I fixed those style issues and rebase this to the master. Once Travis CI turns green, this can go in. Feel free to push the merge button if you want.

@milroy milroy merged commit 64de044 into flux-framework:master Oct 2, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@milroy

This comment has been minimized.

Copy link
Member

milroy commented Oct 2, 2019

@dongahn and I discussed this PR, and since it's relatively small I merged it on behalf of the other reviewers.

@dongahn

This comment has been minimized.

Copy link
Contributor Author

dongahn commented Oct 2, 2019

Thanks @milroy!

@SteVwonder

This comment has been minimized.

Copy link
Member

SteVwonder commented Oct 3, 2019

Thanks @milroy and @dongahn!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.