Skip to content

fix: Improve input handling for input_signature#1698

Merged
gs-olive merged 1 commit intopytorch:mainfrom
gs-olive:input_signature_input_handling
Feb 28, 2023
Merged

fix: Improve input handling for input_signature#1698
gs-olive merged 1 commit intopytorch:mainfrom
gs-olive:input_signature_input_handling

Conversation

@gs-olive
Copy link
Copy Markdown
Contributor

@gs-olive gs-olive commented Feb 25, 2023 •
edited
Loading

Description

  • Improve error messaging for input signature arguments by verifying the nesting depth of the object is within the bounds specified in the TorchScript compiler
  • Throw user-error more quickly to avoid less-interpretable error messages later on

Relevant GraphInputs code:

TensorRT/core/ir/GraphInputs.cpp

Lines 42 to 54 in 5fa6374

if (level == 0) { // a single value like A
collection_inputs.resize(1);
collection_inputs[0].push_back(cur_input);
} else if (level == 1) { // like A in [A, A] or [(B, B), A]
collection_inputs[index].push_back(cur_input);
} else if (level == 2) { // like A in [(A, A), C]
collection_inputs[index].push_back(cur_input);
} else { // only support 2 level
LOG_ERROR(
"Input nesting depth exceeds currently supported depth (3), use 1 level: [A, B], or 2 level: [A, (B, C)]");
}
}
}

Fixes #1595

Type of change

  • Improved input handling + error messaging

Checklist:

  • [ x ] My code follows the style guidelines of this project (You can use the linters)
  • [ x ] I have performed a self-review of my own code
  • [ x ] I have commented my code, particularly in hard-to-understand areas and hacks
  • [ x ] I have made corresponding changes to the documentation
  • [ x ] I have added tests to verify my fix or my feature
  • [ x ] New and existing unit tests pass locally with my changes
  • [ x ] I have added the relevant labels to my PR in so that relevant reviewers are notified

@gs-olive
fix: Improve input handling for input_signature
285018a 
- Improve error messaging for input signature arguments by verifying the
nesting depth of the object is within the bounds specified in the
TorchScript compiler
- Throw user-error more quickly to avoid less-interpretable error
messages later on
@gs-olive gs-olive requested a review from narendasan February 25, 2023 00:01
@gs-olive gs-olive self-assigned this Feb 25, 2023
@github-actions github-actions bot added component: api [Python] Issues re: Python API component: api [C++] Issues re: C++ API labels Feb 25, 2023
narendasan
narendasan approved these changes Feb 27, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@narendasan narendasan left a comment

Choose a reason for hiding this comment

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

Is 2 arbitrary or is there some sort of technical depth limit? If possible i think it would be nice to support deeper nesting like 3-4 but not a massive deal. LGTM

@gs-olive
Copy link
Copy Markdown
Contributor Author

gs-olive commented Feb 28, 2023

Is 2 arbitrary or is there some sort of technical depth limit?

The choice of maximum depth of 2 is based on the data structure used to store the inputs: std::vector<std::vector<Input>>. While this structure could be expanded via adding more nested vector structures, a more robust solution would be desirable (supporting arbitrarily complex collections).

@gs-olive gs-olive merged commit 12db9e8 into pytorch:main Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@narendasan narendasan narendasan approved these changes

Assignees

@gs-olive gs-olive

Labels

cla signed component: api [C++] Issues re: C++ API component: api [Python] Issues re: Python API

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

🐛 [Bug] Encountered bug when compiling model with nested Input type using input_signature argument

3 participants

@gs-olive @narendasan @facebook-github-bot