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

libjobspec: Initial flux jobspec parser #1201

Merged
merged 6 commits into from Oct 9, 2017

Conversation

Projects
None yet
6 participants
@morrone
Copy link
Contributor

morrone commented Sep 25, 2017

Introduce the first revision of the flux jobspec parser library. It
is written in C++ and requires the C++ YAML parsing library name yaml-cpp.

The library is only built if yaml-cpp version 0.5.1 (or greater) is found.

Include a very rough debug tool called flux-jobspec-valdate. It
will eventually grow into a full featured command line validation tool
for flux jobspec files and streams.

@morrone

This comment has been minimized.

Copy link
Contributor Author

morrone commented Sep 25, 2017

I haven't forgotten the other comments in pull request #1198. But it looks like at least some versions of ubuntu do have libyaml-cpp, so I wanted to see what will happen if I run it through travis at this stage.

More updates still to come.

@morrone morrone added the in progress label Sep 25, 2017

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 25, 2017

Looks like travis may still be missing part of boost?

In file included from /usr/include/yaml-cpp/node/node.h:10:0,
                 from /usr/include/yaml-cpp/yaml.h:13,
                 from ../../../../../src/common/libjobspec/jobspec.hpp:31,
                 from ../../../../../src/common/libjobspec/jobspec.cpp:25:
/usr/include/yaml-cpp/node/ptr.h:10:32: fatal error: boost/shared_ptr.hpp: No such file or directory
 #include <boost/shared_ptr.hpp>

This was just in the first build I looked at, I'll spot check the others as well.

@morrone

This comment has been minimized.

Copy link
Contributor Author

morrone commented Sep 25, 2017

Yes, thanks. It looks like 0.5.1 needed it, and 0.5.2 and later no longer do. I guess they failed to list that dependency in the package. I'll add it to travis manually.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 25, 2017

Coverage Status

Coverage decreased (-0.02%) to 78.157% when pulling bdb2263 on morrone:libjobspec into f7883d9 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Sep 25, 2017

One travis failure due to #1169. I'll restart it.

@morrone

This comment has been minimized.

Copy link
Contributor Author

morrone commented Sep 25, 2017

Won't hurt, but I'm satisfied with the current travis status. I'll have more pull request updates coming anyway.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 27, 2017

Codecov Report

Merging #1201 into master will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1201      +/-   ##
==========================================
+ Coverage   78.21%   78.25%   +0.03%     
==========================================
  Files         158      158              
  Lines       29298    29298              
==========================================
+ Hits        22915    22926      +11     
+ Misses       6383     6372      -11
Impacted Files Coverage Δ
src/common/libflux/message.c 81.48% <0%> (-0.12%) ⬇️
src/common/libkvs/kvs.c 65.62% <0%> (+0.25%) ⬆️
src/modules/kvs/kvs.c 64.26% <0%> (+0.26%) ⬆️
src/broker/module.c 84.07% <0%> (+0.27%) ⬆️
src/common/libkvs/kvs_txn.c 78.14% <0%> (+0.66%) ⬆️
src/common/libflux/response.c 84.55% <0%> (+0.81%) ⬆️
src/broker/modservice.c 81.55% <0%> (+0.97%) ⬆️
src/common/libflux/mrpc.c 86.66% <0%> (+1.17%) ⬆️
src/common/libflux/keepalive.c 93.33% <0%> (+6.66%) ⬆️
@morrone

This comment has been minimized.

Copy link
Contributor Author

morrone commented Sep 27, 2017

Still a work in progress. This update adds some testing. Also some bug fixes.

The jobspec objects now also have stream insertion operators.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Sep 27, 2017

Nice!

BTW, just curious. I noticed that you replaced AX_CXX_COMPILE_STDCXX in configure with CXXFLAGS = $(CXXFLAGS) -std=c++11

Didn't like how AX_CXX_COMPILE_STDCXX adds the c++11 compliance option to the compiler command itself?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 27, 2017

Coverage Status

Coverage decreased (-0.05%) to 78.635% when pulling 5b9e54e on morrone:libjobspec into 6486563 on flux-framework:master.

@morrone

This comment has been minimized.

Copy link
Contributor Author

morrone commented Sep 27, 2017

@dongahn , I think you are looking at an earlier revision of the branch. On Monday I created pull request #1204, and this update (commit 5b9e54e) was rebased onto master after that landed.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Sep 27, 2017

Indeed. Good to see you and I are using the same m4 as me for the C++ standard flag check. Plus, good to see CODE_COVERAGE_CXXFLAGS is also supported. It wasn't obvious. Thanks.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 29, 2017

Coverage Status

Coverage increased (+0.004%) to 78.624% when pulling 10a7a74 on morrone:libjobspec into 96a165c on flux-framework:master.

@morrone morrone added review and removed in progress labels Oct 4, 2017

@morrone morrone requested review from grondo and garlick Oct 4, 2017

@morrone

This comment has been minimized.

Copy link
Contributor Author

morrone commented Oct 4, 2017

Assuming that this passes the tests again, it is ready for another round of reviews. If there are no further changes needed from review, it is ready to land.

@morrone morrone requested a review from dongahn Oct 4, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 4, 2017

Coverage Status

Coverage increased (+0.1%) to 78.728% when pulling bfdcc13 on morrone:libjobspec into 96a165c on flux-framework:master.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 5, 2017

The single large commit should be split. I'd suggest:

  • build: find libyaml
  • travis-ci: add libyaml to travis
  • libjobspec: add initial jobspec parser
  • cmd/flux-jobspec: add jobspec validator
  • t/t0018-jobspec.t: test simple valid/invalid jobspec
  • t/t0018-jobspec.t: test RFC 14 use cases
@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 5, 2017

BTW what happened to the libflux/message change that you thought you needed before?

@morrone

This comment has been minimized.

Copy link
Contributor Author

morrone commented Oct 5, 2017

BTW what happened to the libflux/message change that you thought you needed before?

You were right, it was actually misplaced in the commit; it should have been in the job-ingest commit. Since I've dropped that part from the pull request, it doesn't appear here.

@morrone

This comment has been minimized.

Copy link
Contributor Author

morrone commented Oct 5, 2017

The single large commit should be split. I'd suggest:

Suggest or require? :)

@grondo
Copy link
Contributor

grondo left a comment

Besides a few minor comments and the commit breakup suggested by @garlick, this seems good to me.

Really nice to see all those examples being tested!

@@ -0,0 +1,19 @@
version: 1

This comment has been minimized.

Copy link
@grondo

grondo Oct 5, 2017

Contributor

Is this backup file included by accident?

This comment has been minimized.

Copy link
@morrone

morrone Oct 6, 2017

Author Contributor

Yes, thanks!

per_slot: 1
attributes:
system:
duration: 1 hour

This comment has been minimized.

Copy link
@grondo

grondo Oct 5, 2017

Contributor

Github diff noted that this file is missing a final newline. Not a big deal if it works (or if it is on purpose), but I thought I'd point it out because it is different from all the other yaml files here.

This comment has been minimized.

Copy link
@morrone

morrone Oct 6, 2017

Author Contributor

I want to leave that one as-is.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 5, 2017

Hm, a couple of my review comments got lost. They both involved the flux jobspec-validate command.

  • I like the idea and utility of a flux jobspec command. Was there some reason not to have flux jobspec validate instead of flux jobspec-validate? That would seem to make it easier to add future subcommands to the flux jobspec command.

  • My personal preference would be that flux jobspec validate prefix errors with the command name, but that is not a requirement. E.g. instead of Unable to open file "invalid.yaml" do flux-jobspec-validate: Unable to open file "invalid.yaml".

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 5, 2017

The single large commit should be split. I'd suggest:

Suggest or require? :)

I would also like to see the one large commit split. This is not only in the spirit of the C4.1 requirement that "A patch SHOULD be a minimal and accurate answer to exactly one identified and agreed problem", but also I find it easier to track project history as well as handle merge conflicts when commits are built up in this logical way.

There are cases, though, where splitting a commit is especially onerous -- e.g. when it affects compilation of the project or causes ephemeral test failures that would thwart git bisect.

* std::string, std::istream, or the top document YAML::Node as pre-processed
* by the yaml-cpp library.
*
* When errors are found in the jobspec stream the library will raise the

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 5, 2017

Contributor

Than you for the documentation for this!

std::string type;
struct {
unsigned min;
unsigned max;

This comment has been minimized.

Copy link
@dongahn

dongahn Oct 5, 2017

Contributor

From the perspective of users of the jobspec library, it would be helpful to know what value the max field is going to be when only min is given. In this case, I would interpret that as "all available resources," but knowing what specific value I should test this condition on should be extremely helpful. Can this be documented somewhere? I don't believe this belongs to the spec itself but somewhere in this header file should suffice.

This comment has been minimized.

Copy link
@morrone

morrone Oct 6, 2017

Author Contributor

One can't specify only min at this point. That comment can be added when that change is made in the future. First we need to change the rfc 14.

Split complete

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 6, 2017

But ultimately it seemed to make more sense to allow the "flux" command to handle multiple stages of spaces rather than just one. The spelling of flux-jobspec-validate is designed to work with that enhancement in the future.

That's not how our commands work in general, so very likely when you propose flux-jobspec-foo, you will get a review comment to consolidate the jobspec sub-commands.

In fact, does this validator need to be installed as a user-facing command? Perhaps it is more appropriate as a test driver under t/jobspec.

I'm going to split it because it isn't worth fighting over

You make some reasonable points, but It's actually easier to review in smaller, topical chunks (not taken to the extreme of course), and now your focused commit comments make for a better history. For example it's now clear based on the history which test input should be kept in sync with RFC 14 and which could be modified to improve coverage or whatever.

@morrone

This comment has been minimized.

Copy link
Contributor Author

morrone commented Oct 6, 2017

In fact, does this validator need to be installed as a user-facing command? Perhaps it is more appropriate as a test driver under t/jobspec.

Yes, it is meant to be the start of a user-facing command. This will give users an easy way to check their jobspecs during development without having to submit them. Other commands I foresee are "flux jobspec submit" and "flux jobspec simulate".

The simulator would attempt to simulate what is going to happen when the jobspec is executed by flux. I'm not entirely clear on how much detail that could give, or quite how to even express that information yet. :) That idea came from a discussion with Ben, so maybe he'll have some ideas for us. It might be a post-S4 item too.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 6, 2017

The simulator would attempt to simulate what is going to happen when the jobspec is executed by flux. I'm not entirely clear on how much detail that could give, or quite how to even express that information yet. :) That idea came from a discussion with Ben, so maybe he'll have some ideas for us. It might be a post-S4 item too.

That is a great idea!

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 6, 2017

Cool, so sounds like we need flux-jobspec-validate renamed to flux-jobspec with one subcommand (for extra credit set up optparse structure for subcommands), and the output mods suggested by @grondo, and then this is good to go? I'm signing off for a while and I'm fine with whatever you and @grondo work out. Nice work.

@dongahn

This comment has been minimized.

Copy link
Contributor

dongahn commented Oct 6, 2017

This works for me.

@morrone

This comment has been minimized.

Copy link
Contributor Author

morrone commented Oct 6, 2017

Cool, so sounds like we need flux-jobspec-validate renamed to flux-jobspec with one subcommand (for extra credit set up optparse structure for subcommands), and the output mods suggested by @grondo, and then this is good to go?

This already addresses everything @grondo asked for, I believe. (Well, except for introducing an intermediary flux-jobspec command). I would like to leave the command as "flux jobspec-validate" for now. The flux-jobspec or enhanhment to the flux command can be added later without needing to touch the flux-jobspec-validate program.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 6, 2017

The flux-jobspec or enhanhment to the flux command can be added later without needing to touch the flux-jobspec-validate program.

Fine to leave as flux-jobspec-validate for now I think, since there aren't any other jobspec commands yet there's no reason to hide it behind a parent command so far.

However, I'm a bit worried about the future "enhancement to the flux command" -- you're not talking about directly adding jobspec support to the flux(1) command?

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 9, 2017

I'm confused by the last couple comments. Are we in agreement that when further subcommands are added, this command will be renamed to flux-jobspec with a validate subcommand?

I'm OK with merging as is to get the capability integrated as long as we open a bug to rename the command. However, I'm not sure why we wouldn't just rename it now before references appear (say in test scripts) that we will have to change later.

@morrone

This comment has been minimized.

Copy link
Contributor Author

morrone commented Oct 9, 2017

I'm confused by the last couple comments. Are we in agreement that when further subcommands are added, this command will be renamed to flux-jobspec with a validate subcommand?

No, I think it will still be named flux-jobspec-validate. There may be an intermediate command named flux-jobspec that calls it, or the "flux" command itself might be enhanced to handle multiple levels of "-" in subcommands. It is not clear that renaming flux-jobspec-validate will ever be the way to go.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Oct 9, 2017

You are presuming a change to the way flux commands work that hasn't been proposed or discussed, but this is an important PR that shouldn't be held up while we go off on a tangent, IMHO.

Open bug on naming issue and rebase?

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 9, 2017

No, I think it will still be named flux-jobspec-validate. There may be an intermediate command named flux-jobspec that calls it, or the "flux" command itself might be enhanced to handle multiple levels of "-" in subcommands. It is not clear that renaming flux-jobspec-validate will ever be the way to go.

Not sure if you're being purposely mysterious, but if we made flux able to handle multiple levels of - in subcommands you break existing commands that have a - but aren't subcommands, so we can probably take that off the table.

I guess it still isn't clear why the jobspec commands are special and can't be handled like all the other existing flux commands. Since you're proposing a change to existing practice, you unfortunately have an implied burden to prove your approach warrants the change.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 9, 2017

ut this is an important PR that shouldn't be held up while we go off on a tangent, IMHO.

Open bug on naming issue and rebase?

Sorry, missed your comment @garlick.

I agree.

@morrone

This comment has been minimized.

Copy link
Contributor Author

morrone commented Oct 9, 2017

Not sure if you're being purposely mysterious, but if we made flux able to handle multiple levels of - in subcommands you break existing commands that have a - but aren't subcommands, so we can probably take that off the table.

I'm confused about what part of what I'm saying seems mysterious. Can you point me at a current command that has multiple levels of - and would be broken by what I suggest? There aren't any obvious commands like that in src/cmd or src/cmd/builtins, so that comment is kind of mysterious. :)

I guess it still isn't clear why the jobspec commands are special and can't be handled like all the other existing flux commands. Since you're proposing a change to existing practice, you unfortunately have an implied burden to prove your approach warrants the change.

I don't believe that I was the person to introduce that external consideration into this PR. :) But yes, I'm happy to start a separate issue if that lets this PR land.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 9, 2017

Can you point me at a current command that has multiple levels of - and would be broken by what I suggest?

Oops, sorry, I was thinking of flux comms-stats which has since been renamed. I also used flux mock-imp as a command in the IMP prototype. But, you're correct nothing would currently break, sorry! (It would be nice not to close the door on future commands with - embedded, though)

I guess the mystery is why you are proposing that the current method for adding subcommands isn't sufficient for your use case, and a new method is required.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 9, 2017

I don't believe that I was the person to introduce that external consideration into this PR. :)

Ha, point taken! :-)

I think that suggestion had come when there were two jobspec- prefixed subcommands, jobspec-validate and jobspec-submit.

@morrone

This comment has been minimized.

Copy link
Contributor Author

morrone commented Oct 9, 2017

Issue #1226 discusses possible sub-sub-commands.

@garlick garlick referenced this pull request Oct 9, 2017

Open

Sub-sub-commands #1226

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 9, 2017

Ok. Just needs a rebase on latest master and ready for merge

morrone added some commits Sep 23, 2017

libjobspec: Initial flux jobspec parser
Introduce the first revision of the flux jobspec parser library.  It
is written in C++ and requires the C++ YAML parsing library name yaml-cpp.
cmd: Introduce the flux-jobspec-validate command
Introduce the flux-jobspec-validate command, the first consumer
of the jobspec library, which will allow testing in some other commit.

At this point, flux-jobspec-validate is just a rough jobspec validation tool.
Iti will eventually grow into a full featured command line validation tool
for flux jobspec files and streams.  For now it just exits and returns
an error if the jobspec fails to correctly parse by libjobspec.  On
success, for debugging purposes, it prints out the data again in yaml
format from its internal data structures.
config: Test for existance of yaml-cpp library
Add configure test for the C++ yaml-cpp library.
Include a test for the existance of YAML::Node::Mark(), which
is only available in yaml-cpp 0.5.3 and later.
travis: Add the libyaml-cpp-dev package to the build
Add the libyaml-cpp-dev package to the build.  The libyaml-cpp-dev package
in travis seems to have a missing dependency on the boost library, so we
request that too.
test: Introduce tests of the libjobspec library
Introduce the t0018-jobspec.t test script which tests
the libjobspec library using the flux-jobspec-validate command.
It employs the command on a series of valid and invalid yaml
files making sure that the files are detected as valid or invalid
jobspec files.
test: Add the RFC14 examples as libjobspec valid tests
Take all of the current RFC14 example yaml files and add
them to the valid jobspec tests directory.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 9, 2017

Coverage Status

Coverage increased (+0.03%) to 78.657% when pulling 596a13f on morrone:libjobspec into c43a1f1 on flux-framework:master.

@garlick garlick merged commit 9d3b4b1 into flux-framework:master Oct 9, 2017

4 checks passed

codecov/patch Coverage not affected when comparing c43a1f1...596a13f
Details
codecov/project 78.25% (+0.03%) compared to c43a1f1
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 78.657%
Details

@morrone morrone deleted the morrone:libjobspec branch Oct 10, 2017

@grondo grondo referenced this pull request May 10, 2018

Closed

0.9.0 Release #1479

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