Skip to content

Improve node and node argument name generation - #3649

Merged
wschin merged 5 commits into
masterfrom
wechi/naming
Apr 27, 2020
Merged

Improve node and node argument name generation#3649
wschin merged 5 commits into
masterfrom
wechi/naming

Conversation

@wschin

@wschin wschin commented Apr 23, 2020

Copy link
Copy Markdown
Contributor

This PR adds the efforts in #3638 to master branch. This change has two benefits.

  • it ensures the uniqueness of names at graph level.
  • it avoids the change of names when possible. This is quite annoying if ORT keep renaming variables when, for example, adding a new graph transformer.

Q: Why we don't use a global naming manager?
A:

  • If we move the name generator out of graph, there will be two places we check if a name is usable: 1) inside graph's name-generating functions 2) inside name manager's name generator. This is error-prone and largely reduces readability.
  • Having a global name manager implies that manager knows all the names. However, the implication is not correct. The global name manager is not aware of pre-stored names such as initializer names in ONNX model.
  • The above two points may be resolved with a 10x large change. Basically, every places which uses names need to be replaced with some functions in the global name manager.

@wschin
wschin requested a review from a team as a code owner April 23, 2020 06:48
Comment thread onnxruntime/core/graph/graph.cc Outdated
Try not to modify base name (#3638)
@wschin
wschin requested a review from pranavsharma April 26, 2020 04:16
@wschin wschin added the core runtime issues related to core runtime label Apr 26, 2020

REG_KERNEL_NONTEMPL(Clip, 12, Clip, float, double, int8_t, uint8_t, int64_t, uint64_t);

template<typename T>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If Eigen is put in clip.h, it will be included in CUDA kernel's header and trigger a NVCC bug.


// Strings which have been used as node names.
// New node name should not conflict with this set.
std::unordered_set<std::string> generated_node_names_;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

generated_node_names_ [](start = 34, length = 21)

some models seem to have no node names. Will that still be supported?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If the function is called to generate names form them, yes. For example, it will not generate "" if "" is a node name in the model file.

@ke1337 ke1337 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

:shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core runtime issues related to core runtime

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants