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

Generic model builder and inference engine #3415

Closed
wants to merge 56 commits into from

Conversation

@wyessen
Copy link
Contributor

wyessen commented Aug 14, 2019

Quite a bit to chew on in this PR. The main features are:

  • Generic model builder (x-model-builder)
  • Generic inference engine (x-infer)
  • Documentation for the generic model builder and the generic inference engine
wyessen added 30 commits Jun 2, 2019
…nference-engine to integration/xperi-glow-stable

* commit '6ec98623d7db2546b330462c6de7fb03c93336c2':
  Added the generic inference engine.
…odel-builder to integration/xperi-glow-stable

* commit '657ed9b117f9ede04324eaafe565877830ef248d':
  Added the generic model builder.
…rmance-metrics to integration/xperi-glow-stable

* commit 'fb6af78ad7dd91b285f3253ea63674f0336b5847':
  Bug fix.
  Finished performance monitoring code.
  Implemented performance monitoring.
…rmance-metrics to integration/xperi-glow-stable

* commit '00c8071503bdbd866eb8f4deb1ee3b1f1d508477':
  Condition performance monitoring/output on the -p flag.
  Bug fix.
@stale stale bot added the stale_will_be_closed label Sep 7, 2019
@wyessen

This comment has been minimized.

Copy link
Contributor Author

wyessen commented Sep 8, 2019

@jackm321 Please let me know if you need anything from me that could help you with the review.

@stale stale bot removed the stale_will_be_closed label Sep 8, 2019
Copy link
Contributor

jackm321 left a comment

Hi wyessen, sorry for the slow review. I've taken a first pass over this and it looks awesome and I have some general comments in addition to the ones in the code.

  • We should clang format this and try to make the formatting similar to the rest of the codebase

  • The copyright headers need to be changed to be the same as the regular glow copyright headers

  • Naive question but do we need this to be in c? Can we just use C++ like the rest of glow

  • There should be more comments generally like rest of glow on methods, struct definitions, etc.

  • Can we add some tests for this?


const struct argp_option options[] = {
{"output", 'o', "FILE", 0, "Output to binary FILE instead of standard output" },
{"infile", 'i', "FILE", 0, "Input from FILE" },

This comment has been minimized.

Copy link
@jackm321

jackm321 Sep 10, 2019

Contributor

alignment


### A Note on the Initial Contribution

The initial contribution of the `x-inference-engine` (as well as the corresponding documentation) was made as part of the open source contribution initiative by the XPERI Corporation (xperi.com).

This comment has been minimized.

Copy link
@jackm321

jackm321 Sep 10, 2019

Contributor

@likethesky this is ok right?

This comment has been minimized.

Copy link
@likethesky

likethesky Sep 11, 2019

Contributor

Okay; a bit unusual, but allowable. Doesn't change the grant of copyright, or the Glow license in any way. I'd generally discourage these notations, just because they slow us down a bit to confirm wording w/legal, but if they want to make sure their name is in here somewhere, counsel says okay.

This comment has been minimized.

Copy link
@wyessen

wyessen Sep 12, 2019

Author Contributor

Okay; a bit unusual, but allowable. Doesn't change the grant of copyright, or the Glow license in any way. I'd generally discourage these notations, just because they slow us down a bit to confirm wording w/legal, but if they want to make sure their name is in here somewhere, counsel says okay.

@likethesky Understood. Our legal team is OK with us changing the headers to what Glow uses everywhere else. One question, though: are we allowed to identify ourselves somewhere following the source code legal headers? Something like the following, perhaps:

/* Contributed by Xperi Corporation on [date] */

So perhaps the entire header could look like this:

/**
 * Copyright (c) 2017-present, Facebook, Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 *
 * Contributed by Xperi Corporation on [date] 
 */

It doesn't really matter where we identify ourselves, but it feels that somewhere in the header is the most natural place. Thanks!

@@ -0,0 +1,50 @@
/** Copyright 2019 Xperi Corporation.

This comment has been minimized.

Copy link
@jackm321

jackm321 Sep 10, 2019

Contributor

@likethesky correct me if I'm wrong, these should match the copyright headers in the rest of the repo

This comment has been minimized.

Copy link
@likethesky

likethesky Sep 10, 2019

Contributor

Yes, by signing the CLA, all contributors are agreeing to that and although of course anything you write is by definition copyright by the author, the controlling agreement is the CLA and the license that Glow is provided under. So, yes, in order to avoid confusion, all copyright headers should match what we are using in the rest of Glow. Thanks Jack for making sure that's the case--and thank you very kindly @wyessen for your contribution(s)!

This comment has been minimized.

Copy link
@wyessen

wyessen Sep 11, 2019

Author Contributor

Thanks @jackm321 for the review! I'll be getting to the technical points you made shortly.

@likethesky On legal matters: our contribution is of course provided under the same license as Glow, and we did sign the CLA. The only thing that is different in the headers is the "Copyright 2019 Xperi Corporation" rather than "Copyright ... Facebook...".

So my question is whether we should change our header only to avoid confusion, or there is legal pressure? Just trying to gather enough info before I kick this over to the relevant parties on our end. Thanks!

This comment has been minimized.

Copy link
@likethesky

likethesky Sep 11, 2019

Contributor

I checked w/legal before replying the first time. So yes, some legal oversight has occurred and that's their rec. Of course, the Apache 2.0 license allows anyone to copy/use any of this code at any time, so you guys are protected. Also, I probably misspoke or wasn't totally clear in my statement above, when you "write" anything, it's copyright you, but once you contribute it under our CLA, the copyright is granted to Facebook. See under "2. Grant of Copyright License" here: https://code.facebook.com/cla/

Hope that helps. So yes, needs to be (c) Facebook, please use the standard copyright used throughout Glow (and I believe every other of the thousand+ OSS projects we have). Cheers!

int reset_perf_statistics(struct PerfData *pd);
int read_perf_statistics(struct PerfData *pd);

#endif

This comment has been minimized.

Copy link
@jackm321

jackm321 Sep 10, 2019

Contributor

add comments indicating which ifdef these correspond to

int
stop_perf_monitoring(struct PerfData *pd)
{
if (pd->fd >= 0)

This comment has been minimized.

Copy link
@jackm321

jackm321 Sep 10, 2019

Contributor

add {} around this

}

static void
gatherFiles(std::vector<std::string> &files)

This comment has been minimized.

Copy link
@jackm321

jackm321 Sep 10, 2019

Contributor

comments

}

// Stores the list of files containing input in "files".
gatherFiles(files);

This comment has been minimized.

Copy link
@jackm321

jackm321 Sep 10, 2019

Contributor

maybe just return std::vectorstd::string from this? It would be more clear imo

}

// Are we profiling? If so, spit out the profile.
if (glow::profilingGraph())

This comment has been minimized.

Copy link
@jackm321

jackm321 Sep 10, 2019

Contributor

brackets

llvm::cl::cat(inputLoaderCat));

static std::unique_ptr<glow::ProtobufLoader>
createProtobufLoader(glow::Loader &loader, const glow::TypeRef inputType)

This comment has been minimized.

Copy link
@jackm321

jackm321 Sep 10, 2019

Contributor

comments on these

* limitations under the License.
*/

/* Needed to expose (and silence warnings about implicit declaration of) posix_memalign(). */

This comment has been minimized.

Copy link
@jackm321

jackm321 Sep 10, 2019

Contributor

//

Copy link
Contributor Author

wyessen left a comment

Hi wyessen, sorry for the slow review. I've taken a first pass over this and it looks awesome and I have some general comments in addition to the ones in the code.

  • We should clang format this and try to make the formatting similar to the rest of the codebase

Agreed, will do.

  • Naive question but do we need this to be in c? Can we just use C++ like the rest of glow

Not naive at all. In principle we should be able to rewrite in C++, but let me explain the reasons behind the first choice.

We are potentially targeting embedded hardware for which there might not exist a decent C++ compiler. This decision was also (only slightly) reinforced by the choice of Glow to produce C bundles. So, C was really chosen as the smallest common denominator.

  • There should be more comments generally like rest of glow on methods, struct definitions, etc.

Agreed, will fix.

  • Can we add some tests for this?

Certainly, some rudimentary "sanity" tests can be provided. Perhaps we should make this a separate ticket? It will take me a while to do this, as I'm currently low on bandwidth.

@jackm321

This comment has been minimized.

Copy link
Contributor

jackm321 commented Sep 18, 2019

We are potentially targeting embedded hardware for which there might not exist a decent C++ compiler. This decision was also (only slightly) reinforced by the choice of Glow to produce C bundles. So, C was really chosen as the smallest common denominator.

Ok I see, thank you for explaining.

Certainly, some rudimentary "sanity" tests can be provided. Perhaps we should make this a separate ticket? It will take me a while to do this, as I'm currently low on bandwidth.

Sounds good.

@wyessen

This comment has been minimized.

Copy link
Contributor Author

wyessen commented Sep 26, 2019

@jackm321 I believe I've addressed all your concerns in this latest commit. Please have a look. Thanks!

As agreed, tests will be provided as a separate PR.

@wyessen

This comment has been minimized.

Copy link
Contributor Author

wyessen commented Oct 10, 2019

@jackm321 I believe I've addressed all your concerns in this latest commit. Please have a look. Thanks!

As agreed, tests will be provided as a separate PR.

@jackm321 A very gentle bump :). I have some bandwidth to start working on more additions which will be based on this, so I'd like to know if this is fine. Thanks for all your time!

Copy link
Contributor

jackm321 left a comment

@wyessen sorry, I was out on pto last week, I've taken another look and it looks good to me!
The only things I'd request to change are

  1. also match style for variable and function names for example initConstantWeights instead of init_constant_weights
  2. Add inference_engines/ to format.sh
Copy link

facebook-github-bot left a comment

@jackm321 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@wyessen

This comment has been minimized.

Copy link
Contributor Author

wyessen commented Oct 14, 2019

@wyessen sorry, I was out on pto last week, I've taken another look and it looks good to me!
The only things I'd request to change are

  1. also match style for variable and function names for example initConstantWeights instead of init_constant_weights
  2. Add inference_engines/ to format.sh

Hi Jack,

Great, thanks! Ok, will make those changes.

@wyessen

This comment has been minimized.

Copy link
Contributor Author

wyessen commented Oct 14, 2019

@wyessen sorry, I was out on pto last week, I've taken another look and it looks good to me!
The only things I'd request to change are

  1. also match style for variable and function names for example initConstantWeights instead of init_constant_weights
  2. Add inference_engines/ to format.sh

Hi Jack,

Great, thanks! Ok, will make those changes.

Ok, all done.

Copy link

facebook-github-bot left a comment

@jackm321 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

wyessen added 2 commits Oct 15, 2019
@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Oct 15, 2019

@jackm321 merged this pull request in d9240b1.

@jackm321

This comment has been minimized.

Copy link
Contributor

jackm321 commented Oct 15, 2019

Thanks @wyessen for contributing this!

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.