Merge branch 'master' into jagerrit/_win32_fix

VowpalWabbit · Feb 13, 2020 · 0de9e98 · 0de9e98
2 parents 4123550 + 7cc5b24
commit 0de9e98
Show file tree

Hide file tree

Showing 4 changed files with 182 additions and 32 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -67,6 +67,7 @@ option(BUILD_PYTHON "Add Python targets." Off)
 option(BUILD_DOCS "Add documentation targets." Off)
 option(LTO "Enable Link Time optimization (Requires Release build, only works with clang and linux/mac for now)." Off)
 option(BUILD_SLIM_VW "Add targets for slim version of VW which implements only predict() for a subset of VW reductions." OFF)
+option(RAPIDJSON_SYS_DEP "Override using the submodule for RapidJSON dependency. Instead will use find_package" OFF)
 
 string(TOUPPER "${CMAKE_BUILD_TYPE}" CONFIG)
 
@@ -174,44 +175,49 @@ find_package(ZLIB REQUIRED)
 # This provides the variables such as CMAKE_INSTALL_LIBDIR for installation paths.
 include(GNUInstallDirs)
 
-# Ensure rapidjson submodule is ready
-find_package(Git QUIET)
-if(GIT_FOUND AND EXISTS "${PROJECT_SOURCE_DIR}/.git")
-  # Update submodules as needed
-  option(GIT_SUBMODULE "Check submodules during build" ON)
-  if(GIT_SUBMODULE)
-    message(STATUS "Submodule update")
-    execute_process(COMMAND ${GIT_EXECUTABLE} submodule update --init --recursive
-                    WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
-                    RESULT_VARIABLE GIT_SUBMOD_RESULT)
-    if(NOT GIT_SUBMOD_RESULT EQUAL "0")
-      message(FATAL_ERROR "git submodule update --init failed with ${GIT_SUBMOD_RESULT}, please checkout submodules")
+if(RAPIDJSON_SYS_DEP)
+  # Since EXACT is not specified, any version compatible with 1.1.0 is accepted (>= 1.1.0)
+  find_package(RapidJSON 1.1.0 CONFIG REQUIRED)
+  add_library(RapidJSON INTERFACE)
+  target_include_directories(RapidJSON INTERFACE ${RAPIDJSON_INCLUDE_DIRS})
+else()
+  # Ensure rapidjson submodule is ready
+  find_package(Git QUIET)
+  if(GIT_FOUND AND EXISTS "${PROJECT_SOURCE_DIR}/.git")
+    # Update submodules as needed
+    option(GIT_SUBMODULE "Check submodules during build" ON)
+    if(GIT_SUBMODULE)
+      message(STATUS "Submodule update")
+      execute_process(COMMAND ${GIT_EXECUTABLE} submodule update --init --recursive
+                      WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+                      RESULT_VARIABLE GIT_SUBMOD_RESULT)
+      if(NOT GIT_SUBMOD_RESULT EQUAL "0")
+        message(FATAL_ERROR "git submodule update --init failed with ${GIT_SUBMOD_RESULT}, please checkout submodules")
+      endif()
     endif()
   endif()
-endif()
-
-if(NOT EXISTS "${PROJECT_SOURCE_DIR}/rapidjson/CMakeLists.txt")
-  message(FATAL_ERROR "The submodules were not downloaded! GIT_SUBMODULE was turned off or failed. Please update submodules and try again.")
-endif()
 
-add_library(rapidjson INTERFACE)
-target_include_directories(rapidjson INTERFACE
-  $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/rapidjson/include>
-  $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>
-)
+  if(NOT EXISTS "${PROJECT_SOURCE_DIR}/rapidjson/CMakeLists.txt")
+    message(FATAL_ERROR "The submodules were not downloaded! GIT_SUBMODULE was turned off or failed. Please update submodules and try again.")
+  endif()
+  add_library(RapidJSON INTERFACE)
+  target_include_directories(RapidJSON INTERFACE
+    $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/rapidjson/include>
+    $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>
+  )
 
-if(VW_INSTALL)
-  install(
-    TARGETS rapidjson
-    EXPORT VowpalWabbitConfig)
+  if(VW_INSTALL)
+    install(
+      TARGETS RapidJSON
+      EXPORT VowpalWabbitConfig)
 
-  install(
-    DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/rapidjson/include/rapidjson/
-    DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/rapidjson
-  )
+    install(
+      DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/rapidjson/include/rapidjson/
+      DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/rapidjson
+    )
+  endif()
 endif()
 
-
 add_subdirectory(explore)
 add_subdirectory(cluster)
 add_subdirectory(library)

diff --git a/test/unit_test/dsjson_parser_test.cc b/test/unit_test/dsjson_parser_test.cc
@@ -276,3 +276,70 @@ BOOST_AUTO_TEST_CASE(parse_dsjson_cb_as_ccb)
   VW::finish_example(*vw, examples);
   VW::finish(*vw);
 }
+
+BOOST_AUTO_TEST_CASE(parse_dsjson_cb_with_nan)
+{
+  std::string json_text = R"(
+{
+    "_label_cost": "NaN",
+    "_label_probability": "NaN",
+    "_label_Action": 2,
+    "_labelIndex": 1,
+    "o": [
+        {
+            "v": "NaN",
+            "EventId": "123",
+            "ActionTaken": false
+        }
+    ],
+    "Timestamp": "2020-01-15T16:23:36.8640000Z",
+    "Version": "1",
+    "EventId": "abc",
+    "a": [
+        2,
+        1,
+        0
+    ],
+    "c": {
+        "shared_feature":1.0,
+        "_multi": [
+            {
+                "id": "a"
+            },
+            {
+                "id": "b"
+            },
+            {
+                "id": "c"
+            }
+        ]
+    },
+    "p": [
+        "NaN",
+        "NaN",
+        "NaN"
+    ]
+}
+)";
+
+  auto vw = VW::initialize("--dsjson --cb_adf --no_stdin --quiet", nullptr, false, nullptr, nullptr);
+  auto examples = parse_dsjson(*vw, json_text);
+
+  BOOST_CHECK_EQUAL(examples.size(), 4);
+
+  // Shared example
+  BOOST_CHECK_EQUAL(examples[0]->l.cb.costs.size(), 1);
+  BOOST_CHECK_CLOSE(examples[0]->l.cb.costs[0].probability, -1.f, FLOAT_TOL);
+  BOOST_CHECK_CLOSE(examples[0]->l.cb.costs[0].cost, FLT_MAX, FLOAT_TOL);
+
+  // Action examples
+  BOOST_CHECK_EQUAL(examples[1]->l.cb.costs.size(), 0);
+  BOOST_CHECK_EQUAL(examples[2]->l.cb.costs.size(), 1);
+  BOOST_CHECK_EQUAL(examples[3]->l.cb.costs.size(), 0);
+
+  BOOST_CHECK_EQUAL(std::isnan(examples[2]->l.cb.costs[0].probability), true);
+  BOOST_CHECK_EQUAL(std::isnan(examples[2]->l.cb.costs[0].cost), true);
+  BOOST_CHECK_EQUAL(examples[2]->l.cb.costs[0].action, 2);
+  VW::finish_example(*vw, examples);
+  VW::finish(*vw);
+}
diff --git a/vowpalwabbit/CMakeLists.txt b/vowpalwabbit/CMakeLists.txt
@@ -68,7 +68,7 @@ target_link_libraries(vw
     # Workaround an issue where RapidJSON needed to be exported tom install the target. This is
     # actually a private dependency and so do not "link" when processing targets for installation.
     # https://gitlab.kitware.com/cmake/cmake/issues/15415
-    $<BUILD_INTERFACE:rapidjson>)
+    $<BUILD_INTERFACE:RapidJSON>)
 
 add_library(VowpalWabbit::vw ALIAS vw)
 

diff --git a/vowpalwabbit/parse_example_json.h b/vowpalwabbit/parse_example_json.h
@@ -33,6 +33,7 @@
 #include "vw_string_view.h"
 #include <algorithm>
 #include <vector>
+#include <limits>
 
 // portability fun
 #ifndef _WIN32
@@ -197,6 +198,50 @@ class LabelObjectState : public BaseState<audit>
     return this;
   }
 
+  BaseState<audit>* String(Context<audit>& ctx, const char* str, rapidjson::SizeType /* len */, bool) override
+  {
+    if (_stricmp(str, "NaN") != 0)
+    {
+      ctx.error() << "Unsupported label property: '" << ctx.key << "' len: " << ctx.key_length << ". The only string value supported in this context is NaN.";
+      return nullptr;
+    }
+
+    // simple
+    if (!_stricmp(ctx.key, "Label"))
+    {
+      ctx.ex->l.simple.label = std::numeric_limits<float>::quiet_NaN();
+      found = true;
+    }
+    else if (!_stricmp(ctx.key, "Initial"))
+    {
+      ctx.ex->l.simple.initial = std::numeric_limits<float>::quiet_NaN();
+      found = true;
+    }
+    else if (!_stricmp(ctx.key, "Weight"))
+    {
+      ctx.ex->l.simple.weight = std::numeric_limits<float>::quiet_NaN();
+      found = true;
+    }
+    // CB
+    else if (!_stricmp(ctx.key, "Cost"))
+    {
+      cb_label.cost = std::numeric_limits<float>::quiet_NaN();
+      found_cb = true;
+    }
+    else if (!_stricmp(ctx.key, "Probability"))
+    {
+      cb_label.probability = std::numeric_limits<float>::quiet_NaN();
+      found_cb = true;
+    }
+    else
+    {
+      ctx.error() << "Unsupported label property: '" << ctx.key << "' len: " << ctx.key_length;
+      return nullptr;
+    }
+
+    return this;
+  }
+
   BaseState<audit>* Float(Context<audit>& ctx, float v) override
   {
     // simple
@@ -312,6 +357,18 @@ struct LabelSinglePropertyState : BaseState<audit>
     return ctx.previous_state;
   }
 
+  BaseState<audit>* String(Context<audit>& ctx, const char* str, rapidjson::SizeType len, bool copy) override
+  {
+    // skip "_label_"
+    ctx.key += 7;
+    ctx.key_length -= 7;
+
+    if (ctx.label_object_state.String(ctx, str, len, copy) == nullptr)
+      return nullptr;
+
+    return ctx.previous_state;
+  }
+
   BaseState<audit>* Uint(Context<audit>& ctx, unsigned v) override
   {
     // skip "_label_"
@@ -887,6 +944,26 @@ class ArrayToVectorState : public BaseState<audit>
     return this;
   }
 
+  BaseState<audit>* String(
+      Context<audit>& ctx, const char* str, rapidjson::SizeType /*length*/, bool /* copy */) override
+  {
+    if (_stricmp(str, "NaN") != 0)
+    {
+      ctx.error() << "The only supported string in the array is 'NaN'";
+      return nullptr;
+    }
+
+    output_array->push_back(std::numeric_limits<T>::quiet_NaN());
+
+    if (!has_seen_array_start)
+    {
+      has_seen_array_start = false;
+      return return_state;
+    }
+
+    return this;
+  }
+
   BaseState<audit>* Uint(Context<audit>& /* ctx */, unsigned f) override
   {
     output_array->push_back(static_cast<T>(f));