[Runtime] Add config file for Device Config used in Runtime. #2978

beicy · 2019-05-24T18:13:05Z

Summary:
Enable Loader to get device configs from a config yaml file.
Now there are 2 ways in Loader to get the DeviceConfigs:

If the config file is provided, use the file;
otherwise, use the "num-devices" which is 1 by default.

Documentation:

Test Plan:
The config file example is :

---
- name:     Device1
  kindName: CPU
  parameters: |
    "platformID":"1"
    "deviceID" : "0"
- name:     Device2
  kindName: CPU
  parameters: |
    "platformID":"1"
...

The command line:
./bin/text-translator -m "en2gr" -cpu -cpu-memory=500000 -load-device-configs="configs.yaml" <<< "I love music ."

[Optional Fixes #issue]

Please see a detailed explanation of how to fill out the fields in the relevant sections in PULL_REQUEST.md.

rdzhabarov · 2019-05-24T18:25:34Z

lib/Quantization/Serialization.cpp

@@ -35,11 +35,33 @@ template <> struct MappingTraits<glow::NodeQuantizationInfo> {
  }
 };

+template <> struct BlockScalarTraits<glow::MultiLineStr> {


that does not belong to quantization/serialization. Could you move that to a separate configs, lib/configs

also add a test for this.

opti-mix

This is a very useful functionality! Thanks for working on this.

Questions:

Do we really need this multi-line syntax? Or may be YAML has a nicer syntactic alternative for this?
It could be that you do more work than needed and YAML parser can do more for you. @rdzhabarov had some experience with it for quantization. May be he can suggest a better way to use the YAML parser?
Could you add a test with an example YAML file?

opti-mix · 2019-05-24T18:28:21Z

tools/loader/Loader.cpp

+/// Helper to get the BackendKind type from a backend's \p name. Need to be
+/// updated if a new backend comes.
+static BackendKind getBackendKindFromName(std::string &name) {
+  if (name == "CPU")


LLVM has llvm::StringMap for it.

opti-mix · 2019-05-24T18:28:42Z

tools/loader/Loader.cpp

+}
+
+/// Helper to get the parameters in DeviceConfig from \p str. The \p str has
+/// multiply lines, and each line with this format : "str1" : "str2".


mutiply->multiple.

beicy · 2019-05-24T19:39:00Z

Do we really need this multi-line syntax? Or may be YAML has a nicer syntactic alternative for this?

It could be that you do more work than needed and YAML parser can do more for you. @rdzhabarov had some experience with it for quantization. May be he can suggest a better way to use the YAML parser?

Discussed with @rdzhabarov about the yaml file. So far, we haven't found a better way...

gcatron

This looks great! Thanks for doing it. Could you add a test to the expensive tests? It might require checking in an example config file but I think that would be useful to have anyway.

beicy · 2019-05-28T18:22:03Z

Added an unit test for checking yaml file format, and an expensive test for the whole flow.

gcatron · 2019-05-28T19:07:05Z

tests/runtime/configs.yaml

@@ -0,0 +1,12 @@
+---
+- name:     Device1
+  kindName: CPU


minor nit, if we are going to use a CPU maybe rename the parameters so it's clear they are test parameters, or put a comment that these parameters don't mean anything to the CPU backend.

nadavrot · 2019-05-28T20:09:49Z

include/glow/Support/Support.h

+  std::string str;
+};
+
+struct DeviceConfigHelper {


Should this be in "Support.h" or some other header file that is related to device and device configurations?

It is only used in Support.cpp for reading yaml file.

include/glow/Support/Support.h

@@ -119,6 +119,26 @@ inline void report(llvm::StringRef str) { report(str.data()); }
 /// can be inspected under debugger.
 std::string legalizeName(llvm::StringRef name);

+struct MultiLineStr {


nadavrot · 2019-05-28T20:11:52Z

tools/loader/Loader.cpp

@@ -271,6 +280,43 @@ static Kinded::Kind getKindFromNodeName(llvm::StringRef nodeName) {
  GLOW_UNREACHABLE("Unknown node name.");
 }

+/// Helper to get the BackendKind type from a backend's \p name. Need to be


It looks like details of the hardware leak into the loader. This means that every time we add a new hardware backend we would need to update this file. Is there a way to avoid it?

@gcatron was working on something simiar.

I had a chat with @rdzhabarov last week related to this. When we move to dynamic backend registration there will be a string:backendKind map that can be used. I think this solution is a temporary one until we have that map.

We already essentially have this map via llvm::cl::opt<BackendKind> ExecutionBackend here in the Loader. If we must have a map like this too, I think we should unify them somehow.

I think we can have a follow up PR for this, since some tests command line maybe changed as well.

nadavrot · 2019-05-28T20:12:59Z

tools/loader/Loader.cpp

-  for (unsigned int i = 0; i < numDevices; ++i) {
-    auto config = llvm::make_unique<runtime::DeviceConfig>(ExecutionBackend);
-    configs.push_back(std::move(config));
+


This adds a lot of logic to a function that is already long. Is there a way to refactor things and make the code more readable?

Got this piece of code into a function.

nadavrot · 2019-05-28T20:14:03Z

tests/unittests/SupportTest.cpp

@@ -18,6 +18,10 @@
 #include "glow/Testing/StrCheck.h"
 #include "gtest/gtest.h"

+#ifndef GLOW_DATA_PATH


What is this?

This is required for the fbcode setup.

Ok. Got it. Thank you.

tools/loader/Loader.cpp

+  std::istringstream f(str.c_str());
+  while (getline(f, s, '\n')) {
+    size_t pos1, pos2, pos3, pos4;
+


facebook-github-bot

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

gcatron · 2019-05-29T22:49:04Z

tools/loader/Loader.cpp

+/// configs from the file. Otherwise, create \p numDevices number of devices
+/// based on \p backendKind.
+static void generateDeviceConfigs(
+    std::vector<std::unique_ptr<runtime::DeviceConfig>> &configs,


nit: can generateDeviceConfigs return the vector of configs? That way we don't have to pass in a reference.

Done! Thanks!

gcatron

LGTM, once we have dynamic backend discovery we should be able to remove hw specific info from the loader.

facebook-github-bot

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

facebook-github-bot · 2019-05-30T20:32:46Z

@beicy merged this pull request in 65df406.

facebook-github-bot · 2019-05-30T20:33:02Z

@beicy merged this pull request in 65df406.

beicy requested a review from gcatron May 24, 2019 18:13

facebook-github-bot added the CLA Signed label May 24, 2019

rdzhabarov reviewed May 24, 2019

View reviewed changes

opti-mix reviewed May 24, 2019

View reviewed changes

beicy force-pushed the temp1 branch from 2b89eac to e7fe751 Compare May 24, 2019 18:50

beicy changed the title ~~[WIP][Runtime] Add config file for Device Config used in Runtime.~~ [Runtime] Add config file for Device Config used in Runtime. May 24, 2019

gcatron reviewed May 28, 2019

View reviewed changes

beicy force-pushed the temp1 branch 2 times, most recently from 2a7eb21 to 2b39787 Compare May 28, 2019 18:21

beicy force-pushed the temp1 branch from 2b39787 to 496cbee Compare May 28, 2019 18:31

gcatron reviewed May 28, 2019

View reviewed changes

nadavrot reviewed May 28, 2019

View reviewed changes

include/glow/Support/Support.h

@@ -119,6 +119,26 @@ inline void report(llvm::StringRef str) { report(str.data()); }

/// can be inspected under debugger.

std::string legalizeName(llvm::StringRef name);

struct MultiLineStr {

This comment was marked as resolved.

Sign in to view

nadavrot reviewed May 28, 2019

View reviewed changes

tools/loader/Loader.cpp Outdated

std::istringstream f(str.c_str());

while (getline(f, s, '\n')) {

size_t pos1, pos2, pos3, pos4;

This comment was marked as resolved.

Sign in to view

beicy force-pushed the temp1 branch 3 times, most recently from 7bb76de to b8ce02e Compare May 29, 2019 17:09

facebook-github-bot reviewed May 29, 2019

View reviewed changes

gcatron reviewed May 29, 2019

View reviewed changes

[Runtime] Add config file for Device Config used in Runtime.

ce98bbe

beicy force-pushed the temp1 branch from b8ce02e to ce98bbe Compare May 29, 2019 23:21

gcatron approved these changes May 30, 2019

View reviewed changes

facebook-github-bot reviewed May 30, 2019

View reviewed changes

facebook-github-bot closed this in 65df406 May 30, 2019

facebook-github-bot added the Merged label May 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Runtime] Add config file for Device Config used in Runtime. #2978

[Runtime] Add config file for Device Config used in Runtime. #2978

beicy commented May 24, 2019 •

edited

rdzhabarov May 24, 2019

rdzhabarov May 24, 2019

opti-mix left a comment

opti-mix May 24, 2019

opti-mix May 24, 2019

beicy commented May 24, 2019

gcatron left a comment

beicy commented May 28, 2019

gcatron May 28, 2019

nadavrot May 28, 2019

beicy May 28, 2019

This comment was marked as resolved.

nadavrot May 28, 2019

gcatron May 28, 2019

jfix71 May 28, 2019

beicy May 29, 2019

nadavrot May 28, 2019

beicy May 29, 2019 •

edited

nadavrot May 28, 2019

beicy May 28, 2019

nadavrot May 28, 2019

This comment was marked as resolved.

facebook-github-bot left a comment

gcatron May 29, 2019 •

edited

beicy May 29, 2019

gcatron left a comment

facebook-github-bot left a comment

facebook-github-bot commented May 30, 2019

facebook-github-bot commented May 30, 2019

[Runtime] Add config file for Device Config used in Runtime. #2978

[Runtime] Add config file for Device Config used in Runtime. #2978

Conversation

beicy commented May 24, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opti-mix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beicy commented May 24, 2019

gcatron left a comment

Choose a reason for hiding this comment

beicy commented May 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beicy May 29, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

facebook-github-bot left a comment

Choose a reason for hiding this comment

gcatron May 29, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gcatron left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented May 30, 2019

facebook-github-bot commented May 30, 2019

beicy commented May 24, 2019 •

edited

beicy May 29, 2019 •

edited

gcatron May 29, 2019 •

edited