fix duplicate names bug in ImporterContext#467
Conversation
|
|
tdp2110
left a comment
tdp2110
left a comment
There was a problem hiding this comment.
Note that if we have N layers with the same name, we'll have to do O(N^2) lookups in the set. This could be avoided at the cost of additional memory by maintaining per-basname counters (similar to original implementation, but a loop of lookups into the used names set would need to be added). If you didn't want to maintain basenames in the case of count one layer names, you could keep the original maps but change lines like
uniqueName = mLayerNameCounts[name] ? (name + "_" + std::to_string(mLayerNameCounts[name])) : name;
to always append a counter, eg
uniqueName = name + "_" + std::to_string(mLayerNameCounts[name]);
I don't think that approach would require keeping track of a set of used names. I don't know what's appropriate for this library.
| mLayerNameCounts; // Keep track of how many times a tensor name shows up, to avoid duplicate naming in TRT. | ||
| std::set<std::string> | ||
| mTensorNames; // keep track of tensor names used so far, to avoid | ||
| // duplicate avoid duplicate naming in TRT. |
There was a problem hiding this comment.
oops. I duplicated duplicate!
|
Is it worth resolving conflicts on this PR or will this just be ignored? |
|
CC @kevinch-nv |
|
FYI LLVM has to solve a similar problem. For example, see https://llvm.org/docs/tutorial/OCamlLangImpl3.html
You can see their implementation of value naming here, which in the case of name conflicts dispatches here. The basic pattern of the uniquification matches what's done here: there's a map of all used names, and they loop with increasing integer suffixes until something isn't in the map. The particular implementation of the map is not a |
| } | ||
|
|
||
| private: | ||
| static std::string freshName(std::set<std::string> &namesSet, |
There was a problem hiding this comment.
Prefer a more verbose function name, such as generateUniqueName
Suppose we have a network with (not all distinct) layer names layer layer_1 layer When ImporterContext sees "layer", it sees it's not in mLayerNameCounts, and sets mLayerNameCounts["layer"] = 1 and adds a TRT layer with name "layer". It then sees "layer_1", concludes it's not in mLayerNameCounts, so it sets mLayerNameCounts["layer_1"] = 1 and adds a TRT layer with name "layer_1". NOW when it sees "layer", it sees that mLayerNameCounts["layer"] == 1, so we produce a "uniqueName" of "layer" + "_" + std::to_string(mLayerNameCounts["layer"] ), ie "layer_1", which is a name conflict for the TRT net. This change keeps track of all inserted names in a set and in the case of duplicates, tries suffix-appended modifications of the duplicated name by ever increasing integers until a name appears which has not been used.
|
I changed the impl since I last pushed this: now the Also, I realize clang-format may have done something weird with the comments in this file. I used the .clang-format from TensorRT (there's no .clang-format in this repo). I tried multiple versions of clang-format (7,8,9) and they all did the comment-wrapping like this (sometimes clang-format version matters!). If this is a problem, I can manually edit these lines. |
* Prefix logging messages with [TRT] (#497) * Add local build instructions (#498) * fix duplicate layer names bug (#446) (#467) Suppose we have a network with (not all distinct) layer names layer layer_1 layer When ImporterContext sees "layer", it sees it's not in mLayerNameCounts, and sets mLayerNameCounts["layer"] = 1 and adds a TRT layer with name "layer". It then sees "layer_1", concludes it's not in mLayerNameCounts, so it sets mLayerNameCounts["layer_1"] = 1 and adds a TRT layer with name "layer_1". NOW when it sees "layer", it sees that mLayerNameCounts["layer"] == 1, so we produce a "uniqueName" of "layer" + "_" + std::to_string(mLayerNameCounts["layer"] ), ie "layer_1", which is a name conflict for the TRT net. This change keeps track of all inserted names in a set and in the case of duplicates, tries suffix-appended modifications of the duplicated name by ever increasing integers until a name appears which has not been used. * Support Dynamic and 3D instanceNormalization (#515) * Add restrictions on multi-input convolutions (#521) * Update resize limitations in TensorRT (#538) Signed-off-by: Kevin Chen <kevinch@nvidia.com> Co-authored-by: Thomas Peters <thomas.d.peters@gmail.com>
4 participants
Signed-off-by: Tom Peters thomas.d.peters@gmail.com
This is the fix I described from #466