vowpalwabbit / reinforcement_learning Goto Github PK

The specified cpprestsdk dependency version does not build with GCC 9.3

# Checkout 2.10.1 version of cpprestsdk
git checkout e8dda215426172cd348e4d6d455141f40768bf47

g++ (Ubuntu 9.3.0-10ubuntu2) 9.3.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Build issue:

[  0%] Building CXX object src/CMakeFiles/cpprest.dir/http/client/http_client.cpp.o
In file included from /home/jack/cpprestsdk/Release/src/pch/stdafx.h:105,
                 from /home/jack/cpprestsdk/Release/src/http/client/http_client.cpp:16:
/home/jack/cpprestsdk/Release/include/cpprest/json.h: In member function ‘web::json::value& web::json::array::operator[](web::json::array::size_type)’:
/home/jack/cpprestsdk/Release/include/cpprest/json.h:974:29: error: implicitly-declared ‘constexpr msl::safeint3::SafeInt<long unsigned int>::SafeInt(const msl::safeint3::SafeInt<long unsigned int>&)’ is deprecated [-Werror=deprecated-copy]
  974 |             if (nlastSize < nMinSize)
      |                             ^~~~~~~~
In file included from /home/jack/cpprestsdk/Release/include/cpprest/details/basic_types.h:29,
                 from /home/jack/cpprestsdk/Release/src/pch/stdafx.h:92,
                 from /home/jack/cpprestsdk/Release/src/http/client/http_client.cpp:16:
/home/jack/cpprestsdk/Release/include/cpprest/details/SafeInt3.hpp:5639:22: note: because ‘msl::safeint3::SafeInt<long unsigned int>’ has user-provided ‘msl::safeint3::SafeInt<T, E>& msl::safeint3::SafeInt<T, E>::operator=(const msl::safeint3::SafeInt<T, E>&) [with T = long unsigned int; E = msl::safeint3::SafeIntInternal::SafeIntExceptionHandler<msl::safeint3::SafeIntException>]’
 5639 |     SafeInt< T, E >& operator =( const SafeInt< T, E >& rhs ) SAFEINT_NOTHROW
      |                      ^~~~~~~~
/home/jack/cpprestsdk/Release/include/cpprest/details/SafeInt3.hpp:6443:34: note:   initializing argument 1 of ‘bool msl::safeint3::operator<(msl::safeint3::SafeInt<U, E>, msl::safeint3::SafeInt<T, E>) [with T = long unsigned int; U = long unsigned int; E = msl::safeint3::SafeIntInternal::SafeIntExceptionHandler<msl::safeint3::SafeIntException>]’
 6443 | bool operator <( SafeInt< U, E > lhs, SafeInt< T, E > rhs ) SAFEINT_NOTHROW
      |                  ~~~~~~~~~~~~~~~~^~~
In file included from /home/jack/cpprestsdk/Release/src/pch/stdafx.h:105,
                 from /home/jack/cpprestsdk/Release/src/http/client/http_client.cpp:16:
/home/jack/cpprestsdk/Release/include/cpprest/json.h:974:29: error: implicitly-declared ‘constexpr msl::safeint3::SafeInt<long unsigned int>::SafeInt(const msl::safeint3::SafeInt<long unsigned int>&)’ is deprecated [-Werror=deprecated-copy]
  974 |             if (nlastSize < nMinSize)
      |                             ^~~~~~~~
In file included from /home/jack/cpprestsdk/Release/include/cpprest/details/basic_types.h:29,
                 from /home/jack/cpprestsdk/Release/src/pch/stdafx.h:92,
                 from /home/jack/cpprestsdk/Release/src/http/client/http_client.cpp:16:
/home/jack/cpprestsdk/Release/include/cpprest/details/SafeInt3.hpp:5639:22: note: because ‘msl::safeint3::SafeInt<long unsigned int>’ has user-provided ‘msl::safeint3::SafeInt<T, E>& msl::safeint3::SafeInt<T, E>::operator=(const msl::safeint3::SafeInt<T, E>&) [with T = long unsigned int; E = msl::safeint3::SafeIntInternal::SafeIntExceptionHandler<msl::safeint3::SafeIntException>]’
 5639 |     SafeInt< T, E >& operator =( const SafeInt< T, E >& rhs ) SAFEINT_NOTHROW
      |                      ^~~~~~~~
/home/jack/cpprestsdk/Release/include/cpprest/details/SafeInt3.hpp:6443:55: note:   initializing argument 2 of ‘bool msl::safeint3::operator<(msl::safeint3::SafeInt<U, E>, msl::safeint3::SafeInt<T, E>) [with T = long unsigned int; U = long unsigned int; E = msl::safeint3::SafeIntInternal::SafeIntExceptionHandler<msl::safeint3::SafeIntException>]’
 6443 | bool operator <( SafeInt< U, E > lhs, SafeInt< T, E > rhs ) SAFEINT_NOTHROW
      |                                       ~~~~~~~~~~~~~~~~^~~
In file included from /home/jack/cpprestsdk/Release/include/cpprest/http_msg.h:28,
                 from /home/jack/cpprestsdk/Release/src/pch/stdafx.h:116,
                 from /home/jack/cpprestsdk/Release/src/http/client/http_client.cpp:16:
/home/jack/cpprestsdk/Release/include/cpprest/containerstream.h: In instantiation of ‘size_t Concurrency::streams::details::basic_container_buffer<_CollectionType>::in_avail() const [with _CollectionType = std::vector<unsigned char>; size_t = long unsigned int]’:
/home/jack/cpprestsdk/Release/include/cpprest/containerstream.h:116:24:   required from here
/home/jack/cpprestsdk/Release/include/cpprest/containerstream.h:124:38: error: implicitly-declared ‘constexpr msl::safeint3::SafeInt<long unsigned int>::SafeInt(const msl::safeint3::SafeInt<long unsigned int>&)’ is deprecated [-Werror=deprecated-copy]
  124 |             return (size_t)(writeend - readhead);
      |                            ~~~~~~~~~~^~~~~~~~~~~
In file included from /home/jack/cpprestsdk/Release/include/cpprest/details/basic_types.h:29,
                 from /home/jack/cpprestsdk/Release/src/pch/stdafx.h:92,
                 from /home/jack/cpprestsdk/Release/src/http/client/http_client.cpp:16:
/home/jack/cpprestsdk/Release/include/cpprest/details/SafeInt3.hpp:5639:22: note: because ‘msl::safeint3::SafeInt<long unsigned int>’ has user-provided ‘msl::safeint3::SafeInt<T, E>& msl::safeint3::SafeInt<T, E>::operator=(const msl::safeint3::SafeInt<T, E>&) [with T = long unsigned int; E = msl::safeint3::SafeIntInternal::SafeIntExceptionHandler<msl::safeint3::SafeIntException>]’
 5639 |     SafeInt< T, E >& operator =( const SafeInt< T, E >& rhs ) SAFEINT_NOTHROW
      |                      ^~~~~~~~
/home/jack/cpprestsdk/Release/include/cpprest/details/SafeInt3.hpp:6053:48: note:   initializing argument 1 of ‘msl::safeint3::SafeInt<T, E> msl::safeint3::SafeInt<T, E>::operator-(msl::safeint3::SafeInt<T, E>) const [with T = long unsigned int; E = msl::safeint3::SafeIntInternal::SafeIntExceptionHandler<msl::safeint3::SafeIntException>]’
 6053 |     SafeInt< T, E > operator -(SafeInt< T, E > rhs) const SAFEINT_CPP_THROW
      |                                ~~~~~~~~~~~~~~~~^~~

Right now RL.Net.Cli requires a recompile to change its operation (since the mode is hardcoded, and others are commented out).

The work here is to enable arguments passed in to it to decide which scenario to run.

The documentation should include info on all of the fields in the config json.

https://vowpalwabbit.org/docs/reinforcement_learning/cpp/latest/api_config.html

Right now it is impossible to validate which messages were dropped (because of sampling) or not sent because of network issues.

Suggestion for quick improvement:

Introduce event sequence identifier as "{first_event_id}/{last_event_id}".

1. Warning message for sampling: 'We dropped X events (Y%) out of "{first_event_id}/{last_event_id}" block'
2. Warning message for sending: 'We failed to send the block "{first_event_id}/{last_event_id}" because of the following HTTP issue'

A .clang-format needs to be created for consistent style. Strongly consider adopting VW's .clang-format for style consistency between codebases.

Recent versions of cpprestsdk on vcpkg (as of 5/26/20) no longer have openssl as a dependency and this is triggering a failure when building rlclientlib.

This happens because rlclientlib\utility\http_authorization.cc depends on openssl for HMAC.

We should address this by doing the following:

• update documentation and build scripts to explicitly include openssl.
• replace the dependency on windows with the OS crypto library (same goes for OSX).

It looks like this line is dead code, since DECISION_SENDER was removed in commit #189 (@olgavrou ?)

Right now sampling in CCB is not deterministic because of seed corruption:
Did quick debugging and I see that random junk is appended to seed string in cb sample:

Didn't have chance to validate if issue is on vw or rl side yet, so creating issue here

• Change extension to .fb to avoid confusion with .fbs that's used by schema files.
• Make it possible to produce joinable events.

Right now only single frame error reporting is supported.
Some stack support is required:
Before:
"Http code error X"
After:
"Cannot download model file:
Http code error X"

I have an intuition that this repo is potentially of immense practical value. But details in the readme are sparse as to what exactly this does.

Would be awesome if an expert were to write a couple paragraphs describing what this does and how.

thank you

As CCB designed evolved, some bits turned out be to redundant and we slipped from removing them.
They should be removed from the code base.

In no particular order:

• We don't need a separate decision sender
  https://github.com/VowpalWabbit/reinforcement_learning/blob/master/rlclientlib/live_model_impl.cc#L281

• We don't need to register a new sender type (it's identical to existing sender types):
  https://github.com/VowpalWabbit/reinforcement_learning/blob/master/rlclientlib/azure_factories.cc#L22

On Linux there are approximately 21k exported symbols for rl.net.native. (See attached file) There should be 10-20.

exported_symbols.txt

Right now LiveModel.Init is doing check of models' azure location, which can lead to unsuccesful initialization due to transient network issues.
We probably should make it more robust and process model update errors in the same place (background thread).

Config defaults must be intuitive and documented.

For example, interaction and observation senders default to event hub, for some reason.
Another example, the event hub senders default to "localhost:8080" as their url - for some reason.

I don't quite understand the justification for those, but the later is actually not nice as it can cause confusion with people running other stuff in such common development time port.

This code is wrong: https://github.com/VowpalWabbit/reinforcement_learning/blob/master/rlclientlib/live_model_impl.cc#L434

You can't call std::sorton the iterator produced by ranking_response. sort requires a random access iterator while ranking_response's iterator is a forward only iterator.

This, of course, it's compiler dependent since those constraints are not enforceable.

This is breaking compilation on recent versions of clang on OSX.

Schema v2 has indexed rewards and we need binding support for it.

RL.Net.Cli started out as a project to test and debug the RL.Net bindings. Over time we added simulator and replay functionality to it. Long-term, we should probably keep the samples grouped together, and out of the simulation/replay tools.

https://github.com/microsoft/cpprestsdk/pull/883/files

Currently the Python bindings are generated using SWIG - and they are quite difficult to get right. Perhaps we should consider replatforming at the same time as we do it in VW to have a consistent story here?

Currently the best documentation for the C# bindings is an example app:

This should be resolved with proper API documentation.

Design:

Expose a Serializer Interface

• General: Get Inteface using a factory
• General: Buffer Alloc (allocation from pool)
• General: Buffer Return (return to pool)
• General: Serialize into buffer using Flatbuffer?

Implement C#/C++ serialization helper for

• buffer = choose_rank (eventid, context, probability array, action array)
• buffer = report_outcome(eventid, reward)

Expose a Logger Interface

• General: Get Interface using factory

C# binding

• Serializer Interface
• Logger Interface

Need Sizing
Create Tasks

In RLClientLib/vw_model/vw_model.cc the default value for MODEL_VW_INITIAL_COMMAND_LINE is changed to CCB. The default behavior has now changed for cb. The compatibility check for APS model s will always fail.

Change the default command line to cb.

#include <live_model.h>

Becomes:

#include <rl/live_model.h>

• Easier intellisense/completion
• Easier to know which headers are for what project
• Convention

Right now the CMake setup is only documented by the CMakeLists.txt files themselves. We should have some basic documentation (such as in the wiki) to describebuild configuration parameters.

The field "IsExplorationEnabled" is unused in the client.json file. This field is used in several examples throughout the codebase and should be removed if it is unused.

There is a naming inconsistency between our documentation and the C# bindings in the LiveModel TryXYZ APIs.

Are there plans for a C header file? Would make writing bindings easier among other things.

Would love to contribute here if it would help.

Steps to repro:

1. Follow the doc https://github.com/VowpalWabbit/reinforcement_learning/blob/master/bindings/python/README.md:
2. Run make after cmake at step 8

Compilation error:

../../include/constants.h:50: Error: 'MODEL_BACKGROUND_REFRESH' is multiply defined in the generated target language module.
../../include/constants.h:9: Error: Previous declaration of 'MODEL_BACKGROUND_REFRESH'

Workaround = rename 1 of the 2 variables named MODEL_BACKGROUND_REFRESH

Right now we have several sets of parameters required for logger initialization in a single plain list:
"interaction.eventhub.host",
"interaction.eventhub.name",
"interaction.eventhub.keyname",
...
"observation.eventhub.host",
"observation.eventhub.name",
"observation.eventhub.keyname",
...
It introduces some amount of copy-paste for all logger implementations.
It would be helpful either to move to more structured config:
"interaction_config": {"eventhub" { "host", "name"}},...
or at least simulate this behavior by accessing to certain keys of config by prefix

When writing tests with the C# bindings, it's convenient to disregard all interactions and observations by sending them to nowhere.

We should provide a blackhole sender/receiver to make it easier for end-users writing tests with rlclientlib.

This is particularly important given we can't register providers from the bindings.

set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${CMAKE_SOURCE_DIR}/cmake/Modules/")
find_package(FlatBuffers REQUIRED)

${CMAKE_SOURCE_DIR} will not work when the project is consumed as as submodule. This should probably be CMAKE_CURRENT_LIST_DIR and the statement moved to the top level file to be with the other dependencies.

The current usage of SafeHandle is not safe.

There are two ways to correctly use it.

The first is to use pinvoke marshalling to safely convert it to an IntPtr.

The second is to use the following pattern:

bool is_valid = false;
try {
  DangerousAddRef (ref is_valid);
  DoStuff(DangerousGetHandle());
} finally {
 if(is_valid)
   DangerousRelease();
}

Doxygen docs are currently hosted at https://microsoft.github.io/vowpal_wabbit/reinforcement_learning/doc/cpp/html/index.html

The docs should be hosted at this repo. Additionally CI should be created to automate the hosting process of this.

There's no variant that throws RLException

This breaks some tooling and is avoidable. The path can be resolved to the absolute path with:

get_filename_component(resolvedIncludeDir "../include"
                       REALPATH BASE_DIR "${CMAKE_CURRENT_SOURCE_DIR}")

It seems to be doing the incorrect thing in C# currently. Two 0 actions were witnessed

Currently the solution file (rl.sln) use VS2017 and the project files and toolset are VS2015. Need to keep them consistent.
a) Upgrade all projects to VS2017 -or-
b) Downgrade solution file to VS2015 -or-
c) Provide VS2015 compatible solution file and a VS2017 compatible solution file.

This happens when creating a configuration object. The following piece of code doesn't work:

using(Configuration config = null) {
  Configuration.TryLoadConfigurationFromJson("", out config, null);
}

C# doesn't allow passing a variable byref if it's defined in an using statement.
This means the user either will not properly dispose it or relie on finalization.

get_defered_action -> get_deferred_action

Right now to make use of error codes in RL.Net's RLException we need to use magic numbers based on the C++-side header file.

It would be good to have the same constants (or an enum, with translation inside of RL.Net).

Maybe something like https://github.com/SunsetQuest/CppHeaderToCSharpConverter can be used.

#45 as a related issue.

Right now it is possible to run a VW model (or any i_model) that supports exploration out of the box by relying on the PMF output to represent an "exploration distribution". It is also possible to do manual exploration by specifying the PMF via the "Passthrough" mechanism.

Running a VW (or other model) and then changing the exploration distribution (to increase action diversity) is difficult: It requires setting up two live_model instances, one of which is correctly configured not to log, running the one, grabbing the output PMF, tweaking it, generating a new JSON query and running it through a "Passthrough" live_model.

It would be nice to be able to instantiate a live model with a custom exploration strategy callback which will be used to tweak the output from i_model before sampling.

Current Python documentation is generated using doxygen and doesn't seem to really outline the Python API. We should instead use autodoc with Sphinx or something similar.

A little while back we needed to add a workaround to the AppVeyor build to delete the installed OpenSSL packages to enable vcpkg to properly build gRPC/OpenSSL (for cpprestsdk), as detailed here.

microsoft/vcpkg#4189 (comment)

The issue has been closed because

I am going to close this issue now because it does seem to be resolved, at least with AppVeyor since their patch was made. enigma-dev/RadialGM#34
If other people continue to have issues with vcpkg and OpenSSL, then they'll need to file a new issue because I am not able to provide any more information about other issues I do not have and it seems to be fixed here.

We should check whether we can remove this workaround.

using Newtonsoft.Json;
using Newtonsoft.Json.Linq;
using Rl.Net;
using System;
using System.IO;
using System.Threading;

namespace rl_lib_repro
{
    class Program
    {
        static void Main(string[] args)
        {
            string modelFilePath = Path.GetTempFileName();
            File.WriteAllBytes(modelFilePath, new byte[0]);

            var configObj = new JObject();
            configObj["AppId"] = "1234";
            configObj["model.backgroundrefresh"] = false;
            configObj["model.source"] = "FILE_MODEL_DATA";
            configObj["model_file_loader.file_name"] = modelFilePath;
            configObj["model_file_loader.file_must_exist"] = true;

            if (!Configuration.TryLoadConfigurationFromJson(JsonConvert.SerializeObject(configObj), out Configuration config))
                throw new Exception("oh no");

            LiveModel lm = new LiveModel(config);
            lm.Init();
            lm.BackgroundError += (a, b) => Console.WriteLine($"got {a} due to {b}");

            string payload = JsonConvert.SerializeObject(JObject.FromObject(new
            {
                Foo = 1,
                _multi = new object[]
       {
                new
                {
                    ActionA = 1,
                },
                new
                {
                    ActionB = 1,
                },
       },
            }));

            using (var ignore = lm.ChooseRank("lalala", payload, ActionFlags.Default))
            {
                // do nothing with the answer
            }

            lm.Dispose();

            Console.WriteLine("done!");
            Console.ReadLine();
        }
    }
}

Run under the debugger and notice that a background Tpp thread will throw an unhandled exception from rl.net.native.

Hi, are you considering adding trace logging support via the python module? A lot of useful error details are currently hidden.

Currently all API methods take an apistatus object if error details are needed as strings. It would be good to have a utility method to convert the error codes into strings.