Glow: [unittest coverage] Improve test coverage for various aspects of glow

Created on 5 May 2018  路  7Comments  路  Source: pytorch/glow

Every master commit invokes coverage build and the results of coverage build can be found: https://fb-glow-assets.s3.amazonaws.com/coverage/coverage-master/index.html

It would be great to improve test coverage for certain low coverage places.

That's a good first issue to get some familiarity with the glow code base, contributions are appreciated.

good first issue help wanted

Most helpful comment

Seems like fun. I would love to help out and get to know this code a little better.

What's most meaningful for everyone in the long run? Tests that hit multiple low coverage areas at once, or modifying some existing tests to target specific low coverage blocks (i.e. switch cases and small functions which haven't seem to have gotten much love)?

All 7 comments

Seems like fun. I would love to help out and get to know this code a little better.

What's most meaningful for everyone in the long run? Tests that hit multiple low coverage areas at once, or modifying some existing tests to target specific low coverage blocks (i.e. switch cases and small functions which haven't seem to have gotten much love)?

thanks @AnthonyOfSeattle for the interest.

It depends on a certain case, sometimes it makes sense to tweak an existing test to hit all the coverage points. Sometimes it's better to express that as a separate test, really on a case by case basis.

Here are few starters options:

Let me know if i can help you anyhow.

Thanks for the suggestions. It seems that it may not be too difficult to get an example like fr2en but with random initialization like the e2e CNN. Here is a initial idea for a simple graph:

/// Fills the tensor \p H with some stable random integers with the seed \p seed
/// and the range [0 .. scale].
static void fillStableRandomIndex(Handle<size_t> H, size_t seed,
                                  int scale = 10) {
  for (size_t i = 0, e = H.size(); i < e; i++) {
    H.raw(i) = int(i * 1921 + seed) % scale;
  }
}

static std::pair<Function *, SaveNode *>
createSimpleGRUForQuantization(Module *M) {
  Function *F = M->createFunction("main");

  unsigned EMBEDDING_SIZE = 10;
  unsigned LANG_SIZE = 10;
  auto *emb = F->getParent()->createVariable(ElemKind::FloatTy,
                         {LANG_SIZE, EMBEDDING_SIZE},
                         "embedding", VisibilityKind::Public,
                         Variable::TrainKind::None);
  fillStableRandomData(emb->getHandle(), 4565, 1);

  unsigned BATCH_SIZE = 5;
  auto *input = F->getParent()->createVariable(ElemKind::IndexTy,
                           {BATCH_SIZE,5},
                           "input", VisibilityKind::Public,
                           Variable::TrainKind::None);
  fillStableRandomIndex(input->getHandle<size_t>(), 7227, 10);

  Node *inputEmbedded = F->createGather("gru.embedding", emb, input);
  Node *inputSlice = F->createSlice("gru.inputSlice",
                    inputEmbedded,
                    {0,0,0},
                    {BATCH_SIZE, 1, EMBEDDING_SIZE});
  Node *reshape = F->createReshape("gru.reshape",
                   inputSlice,
                   {BATCH_SIZE, EMBEDDING_SIZE});
  std::vector<NodeValue> gruOutput;
  F  -> createGRU("gru", {reshape},
          BATCH_SIZE, 10, 10,
          gruOutput);
  SaveNode *SN = F -> createSave("save", gruOutput.back());
  return {F, SN};
}

I used createGRU for convenience/readability but it now seems difficult to initialize the weights/biases. Is there a way that I am missing to pull these out and explicitly initialize them, or would it be better to just re-implement a GRU from fr2en?

@AnthonyOfSeattle either way would work. I think it's OK (and a bit simpler) to use the existing createGRU method from Graph as you'll get the weights initialized to some initial random state (either xavier or broadcasted).

For the e2e quantization test, we do not care about exact weights but care about accuracy loss due to the conversion of fp32 to int8 model.

Hmm. Seems that I was having difficulties keeping the parameters the same between the interpreterEE and backendSpecificEE graphs when allowing implicit initialization. I was able to get a full test working with a manually specified GRU instead. I can create a PR for this.

That sounds good. The only thing, it looks like the TopK node quantization will be missing, can you add that node as well?

nothing actionable left

Was this page helpful?
0 / 5 - 0 ratings