Triton data converter (CMSSW_11_2_0_pre9)

[drankincms]

This is a first attempt at implementing a convertor factory for transforming inputs from one data type to another, useful for running on different servers with different precision without recompile the main producer.

Converters are located in HeterogeneousCore/SonicTriton/plugins/converters/, and currently two trivial converters are included (FloatStandardConverter.cc and Int64StandardConverter.cc) for float and int64_t in addition to an example converter for float to ap_fixed<16,6> (FloatApFixed16Converter.cc).

[drankincms]

All comments are now addressed, and I have tested with TritonImageProducer and the singularity container. @kpedro88 Please let me know if you have comments on the final implementation of std::any to store the container or the byteSize_ checks.

[drankincms]

Ok, all the comments are now addressed, and I have changed things to allow for multiple separate converters.
This probably needs to be synced with parallel developments, since anything using SONIC will now need to have the converter definitions.

[kpedro88]

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.

[kpedro88]

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).

[drankincms]

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.

[kpedro88]

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.

[drankincms]

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

[drankincms]

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.

[kpedro88]

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...)

[drankincms]

Ok, this got lost a bit during the holidays, just to refocus: @kpedro88 you had suggested modifying the name returned by FaaST to include the suggested data converter. Should that be added here before merging (also neglecting the necessary rebase presumably), or were you thinking that this feature would be included in a future PR?

[kpedro88]

@drankincms I think we should strive to include any foreseen and achievable automation in PRs to the official CMSSW repository. Doing everything by hand is fine for expert testing, but it's not really usable for other users in that mode, let alone for production.

It seems like the bulk of the changes will need to be made in the FaaST server code, with just some additional string parsing introduced in SonicTriton. I would definitely prefer to present a complete product for integration in CMSSW.

[drankincms]

Ok, sounds good, Ill get back to this and add the feature now then 👍

[kpedro88]

here's how I would reduce duplication:

1. define a static function of TritonClient:

   static std::pair<std::string, std::string> splitNameConverter(const std::string& fullname) {
     static const std::string indicator("_DataConverter:");
     size_t dcpos = fullname.find(indicator);
     if (dcpos != std::string::npos)
       return {fullname.substr(0,dcpos), fullname.substr(dcpos+indicator.size())};
     else
       return {fullname, ""};
   }

2. use the static function here:

   const auto& [iname, iconverter] = TritonClient::splitNameConverter(iname_full);

3. pass iconverter to curr_input.defaultConverter() below (& do the same for the outputs further down)
4. defaultConverter() just has to check if the input parameter is non-empty, otherwise it tries to get a standard converter (as currently implemented)
5. inConvMap can be removed entirely (maybe not right away if you still want to test things)

[drankincms]

Ok, yes, this is cleaner, Ill make this change now, thanks. Regarding 5), I think we still want to let the user define their own converter. For example, in the case of FACILE the server would be configured to request FloatToApFixed16F6 as the converter, but if a dedicated FACILE for FPGA producer is written, it can use the same server and would need to use a different converter.

[kpedro88]

I can imagine there might be cases where the user wants to provide an alternate converter, but I'm confused about your example. Why would a dedicated producer need to use a different converter for the same server?

[drankincms]

Im envisioning a dedicated FPGA producer that is aware of the ap_fixed<16,6> type that is needed by the server, so it places each input in the data vector already converted, so the converter is basically just a default UINT16 converter.

[kpedro88]

Ah okay, I understand now.

[kpedro88]

@drankincms let me know when FaaST is updated accordingly (we should test this before we put it into CMSSW)

[drankincms]

Ok, just committed the changes for FaaST to work with v2 here: https://github.com/drankincms/FaaST/tree/v2
I can submit a PR to the fastml main repo if you'd like.

I have been testing this client code with this server, not sure if you're envisioning something more official before we put it in CMSSW.

[kpedro88]

yes, let's discuss the FaaST changes more in a PR to that repo.

it would be good if we had some recipe for other people (i.e. CMSSW PR reviewers) to test the code. I'm not sure if this is easy, though...

Eventually, we might even want to distribute Docker and Singularity containers for FaaST, as we do with Triton.

[drankincms]

Sounds good. We can discuss ways we might be able to test, although it might be quite tricky, I don't know if there are many FPGA-connected resources available ATM.

Regarding the PR, do we want to keep separate branches there for v1 and v2 FaaST? I can make a new branch if this is preferable.

[kpedro88]

I don't see a reason to keep separate FaaST branches right now. The first version is tagged. If, for some reason, we need to go back and make a 1.x release, we can spin off a branch at that time.

[drankincms]

Ok, done. fastmachinelearning/FaaST#2