Skip to content

Conversation

sanskarmodi8
Copy link
Contributor

@sanskarmodi8 sanskarmodi8 commented Sep 10, 2024

Adding validation checks to check the input types and display better error messages for the same.
Fixes #135463

cc @albanD @mruberry @jbschlosser @walterddr @mikaylagawarecki @malfet

Copy link

pytorch-bot bot commented Sep 10, 2024

🔗 Helpful Links

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

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

✅ No Failures

As of commit 5834f1e with merge base e004d53 (image):
💚 Looks good so far! There are no failures yet. 💚

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

@sanskarmodi8
Copy link
Contributor Author

@pytorchbot label "topic: not user facing"

@pytorch-bot pytorch-bot bot added the topic: not user facing topic category label Sep 10, 2024
Copy link
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

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

Please add unit tests and restrict your modifications to specific module, as this PR prevents anyone from extending the module and defining in_features as say floating point argument

@sanskarmodi8
Copy link
Contributor Author

Please add unit tests and restrict your modifications to specific module, as this PR prevents anyone from extending the module and defining in_features as say floating point argument

Ok. I am working on these changes

@sanskarmodi8 sanskarmodi8 requested a review from malfet September 10, 2024 19:20
@cpuhrsch cpuhrsch added module: nn Related to torch.nn module: error checking Bugs related to incorrect/lacking error checking release notes: python_frontend python frontend release notes category topic: improvements topic category and removed topic: not user facing topic category labels Sep 10, 2024
@sanskarmodi8
Copy link
Contributor Author

sanskarmodi8 commented Sep 11, 2024

Hi @malfet,
I have made my changes specific to Linear and Bilinear module and implemented unit tests for the same as you requested.

@albanD albanD removed their request for review September 11, 2024 14:27
@albanD albanD added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Sep 11, 2024
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.

change looks good, just need to move the test

@mikaylagawarecki
Copy link
Contributor

looks like a lot of lines were changed due to your linter, can you undo your linting and use the pytorch lintrunner

make setup_lint, make quicklint from your pytorch folder

@sanskarmodi8
Copy link
Contributor Author

sanskarmodi8 commented Sep 11, 2024

looks like a lot of lines were changed due to your linter, can you undo your linting and use the pytorch lintrunner

make setup_lint, make quicklint from your pytorch folder

Thanks for reviewing, I have made the changes accordingly and pushed it.

@sanskarmodi8 sanskarmodi8 force-pushed the fix_issues_like_#135463 branch from 3d80f04 to 03555a0 Compare September 11, 2024 23:33
malfet
malfet previously approved these changes Sep 12, 2024
Copy link
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the fix

@malfet
Copy link
Contributor

malfet commented Sep 12, 2024

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Sep 12, 2024
@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

@pytorchmergebot
Copy link
Collaborator

The merge job was canceled or timed out. This most often happen if two merge requests were issued for the same PR, or if merge job was waiting for more than 6 hours for tests to finish. In later case, please do not hesitate to reissue the merge command
For more information see pytorch-bot wiki.

@malfet malfet changed the title Fixing issues like #135463 Validate input types for torch.nn.Linear and torch.nn.Bilinear Sep 12, 2024
@malfet
Copy link
Contributor

malfet commented Sep 12, 2024

@pytorchbot merge -f "Let's land those right away"

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

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

@sanskarmodi8
Copy link
Contributor Author

sanskarmodi8 commented Sep 13, 2024

@pytorchbot merge -f "Let's land those right away"

Hey @malfet ,
Is the PR is merged, it's not showing in the Pull Requests section in merged filter. It appears only in closed filter.
Screenshot_2024-09-13-07-57-30-14_320a9a695de7cdce83ed5281148d6f19.jpg

@malfet
Copy link
Contributor

malfet commented Sep 13, 2024

Is the PR is merged, it's not showing in the Pull Requests section in merged filter. It appears only in closed filter. !

It was not merged, but it's part of the trunk, see e157ce3

@sanskarmodi8
Copy link
Contributor Author

Is the PR is merged, it's not showing in the Pull Requests section in merged filter. It appears only in closed filter. !

It was not merged, but it's part of the trunk, see e157ce3

Oh, ok.

@malfet
Copy link
Contributor

malfet commented Sep 13, 2024

@pytorchbot revert -m "It's too restrictive, should allow other int-like types"

Copy link

pytorch-bot bot commented Sep 13, 2024

❌ 🤖 pytorchbot command failed:

@pytorchbot revert: error: the following arguments are required: -c/--classification

usage: @pytorchbot revert -m MESSAGE -c
                          {nosignal,ignoredsignal,landrace,weird,ghfirst}

Try @pytorchbot --help for more info.

@malfet
Copy link
Contributor

malfet commented Sep 13, 2024

@pytorchbot revert -m "It's too restrictive, should allow other int-like types, such as numpy.int64" -c weird

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

@pytorchmergebot
Copy link
Collaborator

@sanskarmodi8 your PR has been successfully reverted.

pytorchmergebot added a commit that referenced this pull request Sep 13, 2024
…near` (#135596)"

This reverts commit e157ce3.

Reverted #135596 on behalf of https://github.com/malfet due to It's too restrictive, should allow other int-like types, such as `numpy.int64` ([comment](#135596 (comment)))
@pytorch-bot pytorch-bot bot dismissed malfet’s stale review September 13, 2024 18:07

This PR was reopened (likely due to being reverted), so your approval was removed. Please request another review.

@malfet
Copy link
Contributor

malfet commented Sep 13, 2024

I.e. things like that should work:

>>> torch.nn.Linear(np.array([1, 2, 3])[0], 2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/nshulga/git/pytorch/pytorch/torch/nn/modules/linear.py", line 103, in __init__
    raise TypeError(f"Expected int for in_features but got {type(in_features)}")
TypeError: Expected int for in_features but got <class 'numpy.int64'>
>>> 

Chao1Han pushed a commit to Chao1Han/pytorch that referenced this pull request Sep 20, 2024
…ytorch#135596)

Adding validation checks to check the input types and display better error messages for the same.
Fixes pytorch#135463

Pull Request resolved: pytorch#135596
Approved by: https://github.com/malfet
Chao1Han pushed a commit to Chao1Han/pytorch that referenced this pull request Sep 20, 2024
…near` (pytorch#135596)"

This reverts commit e157ce3.

Reverted pytorch#135596 on behalf of https://github.com/malfet due to It's too restrictive, should allow other int-like types, such as `numpy.int64` ([comment](pytorch#135596 (comment)))
Copy link
Contributor

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

@github-actions github-actions bot added the Stale label Nov 13, 2024
@github-actions github-actions bot closed this Dec 13, 2024
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 module: error checking Bugs related to incorrect/lacking error checking module: nn Related to torch.nn open source release notes: python_frontend python frontend release notes category Reverted Stale topic: improvements topic 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.

Setting a wrong type value instead of a correct integer typed value to in_features or out_features argument of nn.Linear() gets indirect error messages
7 participants