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

Fix the issue described by #106769 #108340

Closed
wants to merge 2 commits into from
Closed

Conversation

FFFrog
Copy link
Collaborator

@FFFrog FFFrog commented Aug 31, 2023

Fixes #106769

Align the behavior of the C++ interface with the Python interface

  1. Remove some checks in C++ frontend api ,which duplicate with below
    void check_rnn_cell_forward_input(const Tensor& input, c10::SymInt input_size) {
    TORCH_CHECK(
    input.sym_size(1) == input_size,
    "input has inconsistent input_size: got ", input.sym_size(1), " expected ", input_size);
    }
    void check_rnn_cell_forward_hidden(const Tensor& input, const Tensor& hx, c10::SymInt hidden_size, c10::SymInt hidden_label) {
    TORCH_CHECK(
    input.sym_size(0) == hx.sym_size(0),
    "Input batch size ", input.sym_size(0), " doesn't match hidden", hidden_label, " batch size ", hx.sym_size(0));
    TORCH_CHECK(
    hx.sym_size(1) == hidden_size,
    "hidden", hidden_label, " has inconsistent hidden_size: got ", hx.sym_size(1), ", expected ", hidden_size);
    }
  2. Add some checks
  3. support 1D
  4. Add Test

@pytorch-bot
Copy link

pytorch-bot bot commented Aug 31, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/108340

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

✅ No Failures

As of commit 73c6576 with merge base c8e72a4 (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@pytorch-bot pytorch-bot bot added the release notes: cpp release notes category label Aug 31, 2023
@FFFrog
Copy link
Collaborator Author

FFFrog commented Aug 31, 2023

@mikaylagawarecki, @albanD, Hi, sorry to bother you guys, please take a look at this, thanks a lot.

Copy link
Contributor

@mikaylagawarecki mikaylagawarecki left a comment

Choose a reason for hiding this comment

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

Thanks!

torch/csrc/api/src/nn/modules/rnn.cpp Show resolved Hide resolved
torch/csrc/api/src/nn/modules/rnn.cpp Show resolved Hide resolved
@mikaylagawarecki mikaylagawarecki added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Sep 1, 2023
Copy link
Contributor

@mikaylagawarecki mikaylagawarecki left a comment

Choose a reason for hiding this comment

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

One more thing -- could we please add some small tests to ensure that the error messages get raised properly

1. Remove some duplicate checks
2. Add some checks
3. support 1D
4. Add Test
@FFFrog
Copy link
Collaborator Author

FFFrog commented Sep 6, 2023

One more thing -- could we please add some small tests to ensure that the error messages get raised properly

Of course, have added some tests and found a tiny bug.

@FFFrog
Copy link
Collaborator Author

FFFrog commented Sep 8, 2023

@mikaylagawarecki , please have a look at this, thank you.

Copy link
Contributor

@mikaylagawarecki mikaylagawarecki left a comment

Choose a reason for hiding this comment

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

Thanks!

@mikaylagawarecki
Copy link
Contributor

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Sep 8, 2023
@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@FFFrog FFFrog deleted the mrl_fix_grucell branch September 9, 2023 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged open source release notes: cpp release notes category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

torch.nn.GRUCell: Segfault by heap buffer overflow
4 participants