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.

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

Introduce VariadicAlias, remove hardcoded alias limits #6106

Merged
merged 4 commits into from
Dec 11, 2020

Conversation

SherlockNoMad
Copy link
Contributor

Description: Introduce VariadicAlias, remove hardcoded alias range limits

@SherlockNoMad SherlockNoMad added the training issues related to ONNX Runtime training; typically submitted using template label Dec 10, 2020
@SherlockNoMad SherlockNoMad requested a review from wschin December 10, 2020 23:55
@SherlockNoMad SherlockNoMad requested a review from a team as a code owner December 10, 2020 23:55
@@ -64,6 +65,10 @@ class KernelDef {
return alias_map_;
}

const nonstd::optional<std::pair<int, int>>& VariadicAlias() const {
return variadic_alias_offsets_;
Copy link
Contributor

Choose a reason for hiding this comment

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

variadic_alias_offsets_ [](start = 11, length = 23)

How would it be used to represent Lamb's schema such as [eta, w0, g0, m0, v0, w1, g1, m1, v1] -> [w0, m0, v0, w1, g1, v1]? I think we need an initial bias and a vector to specify the input-to-output mapping in a peridic pattern. For my example here, the vector is [0, -1, 1, 2] where '-1' mean 'g0' is not mapped to output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lamb is too special a case, I will leave it out, to make the interface clean and simple.

Copy link
Contributor

Choose a reason for hiding this comment

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

Many optimizers may fall into that category, not only Lamb.

wschin
wschin previously approved these changes Dec 11, 2020
Copy link
Contributor

@wschin wschin left a comment

Choose a reason for hiding this comment

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

This PR removes the input-output-length limit on several operators, which is good. I hope we can have a long-term solution for all operators with variadic-input/output list, but if this unblocks a production model, I am fine to merge this PR.

@SherlockNoMad SherlockNoMad merged commit a53f4dd into master Dec 11, 2020
@SherlockNoMad SherlockNoMad deleted the bahuang/alias branch December 11, 2020 18:47
pengwa pushed a commit that referenced this pull request Dec 28, 2020
* Introduce VariadicAlias, remove hardcoded alias limits

* Include optional-lite in winml build

Co-authored-by: Sherlock Huang <bahuang@OrtTrainingDev3.af05slrtruoetgaxwwjv5nsq5e.px.internal.cloudapp.net>
(cherry picked from commit a53f4dd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
training issues related to ONNX Runtime training; typically submitted using template
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants