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.

Sign up for GitHub

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

Triton data converter (CMSSW_11_2_0_pre9) #2

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Triton data converter (CMSSW_11_2_0_pre9) #2

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
2762c02
rebasing to pre9
dsrankin Nov 16, 2020
05f2ca0
formatting and code checks
dsrankin Nov 16, 2020
b4a3f2c
cleanup, removing unused PSet arg for converter
dsrankin Nov 21, 2020
5bade57
using std::any to store converter
dsrankin Nov 23, 2020
d9bf227
adjusting to handle possibility of multiple converters
dsrankin Nov 24, 2020
920ee4f
fixing typo
dsrankin Nov 24, 2020
47fb54c
removing old comment
dsrankin Nov 24, 2020
70a522b
fixing another typo
dsrankin Nov 24, 2020
037c8b0
adding clear for more complicated conversions
dsrankin Nov 30, 2020
90ce2ed
adding override
dsrankin Dec 11, 2020
e653471
fixing bytesize check
dsrankin Dec 11, 2020
6e336a3
removing unnecessary line
dsrankin Dec 11, 2020
8ebdcd0
adding underscore, expanding to multiple converters for inputs and ou…
dsrankin Dec 14, 2020
15c99a4
setting up default behavior
dsrankin Dec 14, 2020
81077f0
a few optimizations
dsrankin Dec 15, 2020
3231ad8
small formatting
dsrankin Dec 15, 2020
daabb94
removing unnecessary defs
dsrankin Dec 15, 2020
2dabb92
removing unnecessary defs
dsrankin Dec 15, 2020
9c14bf5
adding ability to get converter from server
dsrankin Jan 20, 2021
4037115
reducing duplication
dsrankin Jan 20, 2021
9faebc0
change empty check
dsrankin Jan 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions HeterogeneousCore/SonicTriton/BuildFile.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
<use name="FWCore/Utilities"/>
<use name="FWCore/ParameterSet"/>
<use name="FWCore/MessageLogger"/>
<use name="DataFormats/Common"/>
<use name="HeterogeneousCore/SonicCore"/>
<use name="triton-inference-server"/>
<use name="protobuf"/>
Expand Down
43 changes: 43 additions & 0 deletions HeterogeneousCore/SonicTriton/interface/TritonConverterBase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
#ifndef HeterogeneousCore_SonicTriton_TritonConverterBase
#define HeterogeneousCore_SonicTriton_TritonConverterBase

#include "FWCore/ParameterSet/interface/ParameterSet.h"
#include "DataFormats/Common/interface/Handle.h"

#include <string>

template <typename DT>
class TritonConverterBase {
dsrankin marked this conversation as resolved.
Show resolved Hide resolved
//class needs to be templated since the convert functions require the data type, but need to also be virtual, and virtual member function templates are not allowed in C++
public:
TritonConverterBase(const std::string convName)
: converterName_(convName), byteSize_(sizeof(DT)) {}
TritonConverterBase(const std::string convName, size_t byteSize)
: converterName_(convName), byteSize_(byteSize) {}
TritonConverterBase(const TritonConverterBase&) = delete;
virtual ~TritonConverterBase() = default;
TritonConverterBase& operator=(const TritonConverterBase&) = delete;

virtual const uint8_t* convertIn (const DT* in) const = 0;
virtual const DT* convertOut (const uint8_t* in) const = 0;

const int64_t byteSize() const { return byteSize_; }

const std::string& name() const { return converterName_; }

virtual void clear() const {}

private:
const std::string converterName_;
const int64_t byteSize_;
};

#include "FWCore/PluginManager/interface/PluginFactory.h"

template <typename DT>
using TritonConverterFactory = edmplugin::PluginFactory<TritonConverterBase<DT>*()>;

#define DEFINE_TRITON_CONVERTER(input, type, name) DEFINE_EDM_PLUGIN(TritonConverterFactory<input>, type, name)
#define DEFINE_TRITON_CONVERTER_SIMPLE(input, type) DEFINE_EDM_PLUGIN(TritonConverterFactory<input>, type, #type)

#endif
22 changes: 22 additions & 0 deletions HeterogeneousCore/SonicTriton/interface/TritonData.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
#include "FWCore/Utilities/interface/Exception.h"
#include "FWCore/Utilities/interface/Span.h"

#include "FWCore/PluginManager/interface/PluginFactory.h"
#include "HeterogeneousCore/SonicTriton/interface/TritonConverterBase.h"

#include <vector>
#include <string>
#include <unordered_map>
Expand Down Expand Up @@ -69,6 +72,22 @@ class TritonData {
void setResult(std::shared_ptr<Result> result) { result_ = result; }
IO* data() { return data_.get(); }

void setConverterParams(const edm::ParameterSet& conf) {
converterName_ = conf.getParameter<std::string>("converterName");
}

template <typename DT>
std::shared_ptr<TritonConverterBase<DT>> createConverter() const {
using ConverterType = std::shared_ptr<TritonConverterBase<DT>>;
//this construction catches bad any_cast without throwing std exception
if (auto ptr = std::any_cast<ConverterType>(&converter_)) {
} else {
converter_ = ConverterType(TritonConverterFactory<DT>::get()->create(converterName_));
converter_clear_ = std::bind(&TritonConverterBase<DT>::clear, std::any_cast<ConverterType>(converter_).get());
}
return std::any_cast<ConverterType>(converter_);
}

//helpers
bool anyNeg(const ShapeView& vec) const {
return std::any_of(vec.begin(), vec.end(), [](int64_t i) { return i < 0; });
Expand All @@ -93,6 +112,9 @@ class TritonData {
int64_t byteSize_;
std::any holder_;
std::shared_ptr<Result> result_;
mutable std::any converter_;
std::string converterName_;
mutable std::function<void()> converter_clear_;
};

using TritonInputData = TritonData<nvidia::inferenceserver::client::InferInput>;
Expand Down
5 changes: 5 additions & 0 deletions HeterogeneousCore/SonicTriton/plugins/BuildFile.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<library name="HeterogeneousCoreSonicTritonPlugins_converters" file="converters/*.cc">
<use name="HeterogeneousCore/SonicTriton"/>
<use name="hls"/>
<flags EDM_PLUGIN="1"/>
</library>
44 changes: 44 additions & 0 deletions HeterogeneousCore/SonicTriton/plugins/converters/FloatApFixed16Converter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
#include "HeterogeneousCore/SonicTriton/interface/TritonConverterBase.h"

#include <string>
#include "ap_fixed.h"

template <int I>
class FloatApFixed16Converter : public TritonConverterBase<float> {
public:
FloatApFixed16Converter() : TritonConverterBase<float>("FloatApFixed16F"+std::to_string(I)+"Converter", 2) {}

const uint8_t* convertIn(const float* in) const override {
auto temp_vec = std::make_shared<std::vector<ap_fixed<16, I>>>(std::move(this->makeVecIn(in)));
inputHolder_.push_back(temp_vec);
return reinterpret_cast<const uint8_t*>(temp_vec->data());
}
const float* convertOut(const uint8_t* in) const override {
auto temp_vec = std::make_shared<std::vector<float>>(std::move(this->makeVecOut(reinterpret_cast<const ap_fixed<16, I>*>(in))));
outputHolder_.push_back(temp_vec);
return temp_vec->data();
}

void clear() const override {
inputHolder_.clear();
outputHolder_.clear();
}

private:
std::vector<ap_fixed<16, I>> makeVecIn(const float* in) const {
unsigned int nfeat = sizeof(in) / sizeof(float);
std::vector<ap_fixed<16, I>> temp_storage(in, in + nfeat);
return temp_storage;
}

std::vector<float> makeVecOut(const ap_fixed<16, I>* in) const {
unsigned int nfeat = sizeof(in) / sizeof(ap_fixed<16, I>);
std::vector<float> temp_storage(in, in + nfeat);
return temp_storage;
}

mutable std::vector<std::shared_ptr<std::vector<ap_fixed<16, I>>>> inputHolder_;
mutable std::vector<std::shared_ptr<std::vector<float>>> outputHolder_;
};

DEFINE_TRITON_CONVERTER(float, FloatApFixed16Converter<6>, "FloatApFixed16F6Converter");
11 changes: 11 additions & 0 deletions HeterogeneousCore/SonicTriton/plugins/converters/FloatStandardConverter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#include "HeterogeneousCore/SonicTriton/interface/TritonConverterBase.h"

class FloatStandardConverter : public TritonConverterBase<float> {
public:
FloatStandardConverter() : TritonConverterBase<float>("FloatStandardConverter") {}

const uint8_t* convertIn(const float* in) const override { return reinterpret_cast<const uint8_t*>(in); }
const float* convertOut(const uint8_t* in) const override { return reinterpret_cast<const float*>(in); }
};

DEFINE_TRITON_CONVERTER_SIMPLE(float, FloatStandardConverter);
11 changes: 11 additions & 0 deletions HeterogeneousCore/SonicTriton/plugins/converters/Int64StandardConverter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#include "HeterogeneousCore/SonicTriton/interface/TritonConverterBase.h"

class Int64StandardConverter : public TritonConverterBase<int64_t> {
public:
Int64StandardConverter() : TritonConverterBase<int64_t>("Int64StandardConverter") {}

const uint8_t* convertIn(const int64_t* in) const override { return reinterpret_cast<const uint8_t*>(in); }
const int64_t* convertOut(const uint8_t* in) const override { return reinterpret_cast<const int64_t*>(in); }
};

DEFINE_TRITON_CONVERTER_SIMPLE(int64_t, Int64StandardConverter);
27 changes: 27 additions & 0 deletions HeterogeneousCore/SonicTriton/src/TritonClient.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ TritonClient::TritonClient(const edm::ParameterSet& params)
if (!msg_str.empty())
throw cms::Exception("ModelErrors") << msg_str;

const std::vector<edm::ParameterSet>& inputConverterDefs = params.getParameter<std::vector<edm::ParameterSet>>("inputConverters");
dsrankin marked this conversation as resolved.
Show resolved Hide resolved
//setup input map
std::stringstream io_msg;
if (verbose_)
Expand All @@ -90,6 +91,14 @@ TritonClient::TritonClient(const edm::ParameterSet& params)
auto [curr_itr, success] = input_.emplace(
std::piecewise_construct, std::forward_as_tuple(iname), std::forward_as_tuple(iname, nicInput, noBatch_));
auto& curr_input = curr_itr->second;
bool foundConverter = false;
for (const auto converterDef : inputConverterDefs) {
dsrankin marked this conversation as resolved.
Show resolved Hide resolved
if (converterDef.getParameter<std::string>("inputName") == iname) {
curr_input.setConverterParams(converterDef);
foundConverter = true;
}
}
if (!foundConverter) throw cms::Exception("ModelErrors") << "No matching converter definition for input: " << iname;
inputsTriton_.push_back(curr_input.data());
if (verbose_) {
io_msg << " " << iname << " (" << curr_input.dname() << ", " << curr_input.byteSize()
Expand All @@ -101,6 +110,8 @@ TritonClient::TritonClient(const edm::ParameterSet& params)
const auto& v_outputs = params.getUntrackedParameter<std::vector<std::string>>("outputs");
std::unordered_set s_outputs(v_outputs.begin(), v_outputs.end());

const std::vector<edm::ParameterSet>& outputConverterDefs = params.getParameter<std::vector<edm::ParameterSet>>("outputConverters");

//setup output map
if (verbose_)
io_msg << "Model outputs: "
Expand All @@ -113,6 +124,14 @@ TritonClient::TritonClient(const edm::ParameterSet& params)
auto [curr_itr, success] = output_.emplace(
std::piecewise_construct, std::forward_as_tuple(oname), std::forward_as_tuple(oname, nicOutput, noBatch_));
auto& curr_output = curr_itr->second;
bool foundConverter = false;
for (const auto converterDef : outputConverterDefs) {
if (converterDef.getParameter<std::string>("outputName") == oname) {
curr_output.setConverterParams(converterDef);
foundConverter = true;
}
}
if (!foundConverter) throw cms::Exception("ModelErrors") << "No matching converter definition for output: " << oname;
outputsTriton_.push_back(curr_output.data());
if (verbose_) {
io_msg << " " << oname << " (" << curr_output.dname() << ", " << curr_output.byteSize()
Expand Down Expand Up @@ -336,10 +355,18 @@ inference::ModelStatistics TritonClient::getServerSideStatus() const {

//for fillDescriptions
void TritonClient::fillPSetDescription(edm::ParameterSetDescription& iDesc) {
edm::ParameterSetDescription descInConverter;
descInConverter.add<std::string>("converterName");
descInConverter.add<std::string>("inputName");
edm::ParameterSetDescription descOutConverter;
descOutConverter.add<std::string>("converterName");
descOutConverter.add<std::string>("outputName");
edm::ParameterSetDescription descClient;
fillBasePSetDescription(descClient);
descClient.add<std::string>("modelName");
descClient.add<std::string>("modelVersion", "");
descClient.addVPSet("inputConverters", descInConverter);
descClient.addVPSet("outputConverters", descOutConverter);
//server parameters should not affect the physics results
descClient.addUntracked<unsigned>("batchSize");
descClient.addUntracked<std::string>("address");
Expand Down
19 changes: 13 additions & 6 deletions HeterogeneousCore/SonicTriton/src/TritonData.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "HeterogeneousCore/SonicTriton/interface/TritonData.h"
#include "HeterogeneousCore/SonicTriton/interface/triton_utils.h"
#include "HeterogeneousCore/SonicTriton/interface/TritonConverterBase.h"
#include "FWCore/MessageLogger/interface/MessageLogger.h"

#include "model_config.pb.h"
Expand Down Expand Up @@ -116,14 +117,16 @@ void TritonInputData::toServer(std::shared_ptr<TritonInput<DT>> ptr) {
//shape must be specified for variable dims or if batch size changes
data_->SetShape(fullShape_);

if (byteSize_ != sizeof(DT))
throw cms::Exception("TritonDataError") << name_ << " input(): inconsistent byte size " << sizeof(DT)
auto converter = createConverter<DT>();

if (byteSize_ != converter->byteSize())
throw cms::Exception("TritonDataError") << name_ << " input(): inconsistent byte size " << converter->byteSize()
<< " (should be " << byteSize_ << " for " << dname_ << ")";

int64_t nInput = sizeShape();
for (unsigned i0 = 0; i0 < batchSize_; ++i0) {
const DT* arr = data_in[i0].data();
triton_utils::throwIfError(data_->AppendRaw(reinterpret_cast<const uint8_t*>(arr), nInput * byteSize_),
triton_utils::throwIfError(data_->AppendRaw(converter->convertIn(arr), nInput * byteSize_),
name_ + " input(): unable to set data for batch entry " + std::to_string(i0));
}

Expand All @@ -138,7 +141,9 @@ TritonOutput<DT> TritonOutputData::fromServer() const {
throw cms::Exception("TritonDataError") << name_ << " output(): missing result";
}

if (byteSize_ != sizeof(DT)) {
auto converter = createConverter<DT>();

if (byteSize_ != converter->byteSize()) {
throw cms::Exception("TritonDataError") << name_ << " output(): inconsistent byte size " << sizeof(DT)
<< " (should be " << byteSize_ << " for " << dname_ << ")";
}
Expand All @@ -147,14 +152,14 @@ TritonOutput<DT> TritonOutputData::fromServer() const {
TritonOutput<DT> dataOut;
const uint8_t* r0;
size_t contentByteSize;
size_t expectedContentByteSize = nOutput * byteSize_ * batchSize_;
size_t expectedContentByteSize = nOutput * converter->byteSize() * batchSize_;
triton_utils::throwIfError(result_->RawData(name_, &r0, &contentByteSize), "output(): unable to get raw");
if (contentByteSize != expectedContentByteSize) {
throw cms::Exception("TritonDataError") << name_ << " output(): unexpected content byte size " << contentByteSize
<< " (expected " << expectedContentByteSize << ")";
}

const DT* r1 = reinterpret_cast<const DT*>(r0);
const DT* r1 = converter->convertOut(r0);
dataOut.reserve(batchSize_);
for (unsigned i0 = 0; i0 < batchSize_; ++i0) {
auto offset = i0 * nOutput;
Expand All @@ -168,11 +173,13 @@ template <>
void TritonInputData::reset() {
data_->Reset();
holder_.reset();
converter_clear_();
}

template <>
void TritonOutputData::reset() {
result_.reset();
converter_clear_();
}

//explicit template instantiation declarations
Expand Down
4 changes: 4 additions & 0 deletions HeterogeneousCore/SonicTriton/src/pluginFactories.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
#include "HeterogeneousCore/SonicTriton/interface/TritonConverterBase.h"

EDM_REGISTER_PLUGINFACTORY(TritonConverterFactory<float>, "TritonConverterFloatFactory");
EDM_REGISTER_PLUGINFACTORY(TritonConverterFactory<int64_t>, "TritonConverterInt64Factory");
36 changes: 36 additions & 0 deletions HeterogeneousCore/SonicTriton/test/tritonTest_cfg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,40 @@
"TritonGraphProducer": "gat_test",
}

inConvs = {
"TritonImageProducer": cms.VPSet(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To simplify this for users, we might want to add some additional infrastructure:

  1. To handle default cases, we could map the standard converters to the appropriate Triton data type names/enums (see the TritonData constructor). Then these wouldn't need to be specified in the python config explicitly.
  2. We might want to investigate if there's a way to augment the model config and protocols so that, when a FaaST server is used, it could automatically specify (probably via some gRPC message) when an alternate converter needs to be used (and which one).

Item 2 assumes that there would not be a case where the client wants to use a different converter than specified by the model in the repository/on the server.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One idea to avoid a) overloading the meaning of some existing protobuf field (potentially dangerous) or b) extending the proto definitions (maintenance burden):
we could come up with a policy whereby the input and output names in the config.pbtxt are extended to indicate which non-standard converter (if any) is needed. Then these extended names could be handled by the client.
We could try to have the FaaST server perform this extension automatically (in order to keep the same config.pbtxt files for GPU and FPGA versions of a model). But since other model files might need to be different for FPGA usage, changing the config.pbtxt might be okay.

Even with this, it could still be useful to have the option introduced here to specify a converter manually (for testing).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Ok yeah, this is a good suggestion, Ill set this up.

  2. Hmm, perhaps we could use something existing like the platform field in ModelMetadataResponse, and fill it with the server's desired converter. This seems simple enough for FaaST, but I'm not sure how easy this would be with a standard GPU Triton server.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's still only one platform per model, so we would have to pack all the information for every input and output into that one string, which isn't ideal.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, I keep forgetting that other models have multiple inputs/outputs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this now uses defaults successfully when they are available, so 1) is addressed.

For 2), Ok, I missed your suggestion of the model name field. I think the version field might be even more accurate but it's a minor point. But can we enforce a policy whereby we use the field to encode the necessary converter? I'm wondering if you think this is actually feasible or if you were only thinking out loud a bit here.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be a bit more explicit, the most automatic implementation would be:

  1. config.pbtxt retains the normal input/output names (same for GPU and FPGA)
  2. FaaST appends information to the input/output names when returning the ModelMetadataResponse, e.g. "input1_FPGAConverter:foo"
  3. TritonClient knows to check for the presence of "_FPGAConverter:foo"

The question here would be how "foo" is provided for each model input/output. Maybe there could be a separate section of the config.pbtxt; we have to learn more about how Triton parses it and what (if anything) it ignores.

The alternative would be a bit more manual on the user side:

  1. make a separate FPGA copy of the GPU config.pbtxt, and add "_FPGAConverter:foo" to the input/output names.
  2. FaaST uses the FPGA copy of the config.pbtxt (maybe this could also be automated...)

cms.PSet(
converterName = cms.string("FloatStandardConverter"),
inputName = cms.string("gpu_0/data"),
),
),
"TritonGraphProducer": cms.VPSet(
cms.PSet(
converterName = cms.string("FloatStandardConverter"),
inputName = cms.string("x__0"),
),
cms.PSet(
converterName = cms.string("Int64StandardConverter"),
inputName = cms.string("edgeindex__1"),
),
),
}

outConvs = {
"TritonImageProducer": cms.VPSet(
cms.PSet(
converterName = cms.string("FloatStandardConverter"),
outputName = cms.string("gpu_0/softmax"),
),
),
"TritonGraphProducer": cms.VPSet(
cms.PSet(
converterName = cms.string("FloatStandardConverter"),
outputName = cms.string("logits__0"),
),
),
}

if options.producer not in models:
raise ValueError("Unknown producer: "+options.producer)

Expand All @@ -49,6 +83,8 @@
modelVersion = cms.string(""),
verbose = cms.untracked.bool(options.verbose),
allowedTries = cms.untracked.uint32(0),
inputConverters = inConvs[options.producer],
outputConverters = outConvs[options.producer],
)
)
if options.producer=="TritonImageProducer":
Expand Down