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

stop allowing extra positional args in arg parser #10499

Closed
wants to merge 6 commits into from

Conversation

li-roy
Copy link
Contributor

@li-roy li-roy commented Aug 14, 2018

Arg parser allowed additional positional args to be parsed into keyword-only params.

Fixes a couple cases:

  • The positional argument happens to be of the right type, and it just works silently. Now, we fail as expected.
  • The positional argument fails later down the line. Now, we fail at the appropriate time and get a better error message.

Pre-fix:

>>> torch.cuda.LongTensor((6, 0), 1, 1, 0)
tensor([6, 0], device='cuda:1')

Post-fix:

>>> torch.cuda.LongTensor((6, 0), 1, 1, 0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: new() received an invalid combination of arguments - got (tuple, int, int, int), but expected one of:
 * (torch.device device)
 * (torch.Storage storage)
 * (Tensor other)
 * (tuple of ints size, torch.device device)
 * (object data, torch.device device)

Pre-fix:

>>> a = torch.tensor(5)
>>> a.new_zeros((5,5), 0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: new_zeros(): argument 'dtype' (position 2) must be torch.dtype, not int

Post-fix:

>>> a = torch.tensor(5)
>>> a.new_zeros((5,5), 0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: new_zeros() takes 1 positional argument but 2 were given

fixes #8351

@ezyang
Copy link
Contributor

ezyang commented Aug 15, 2018

Tests?

(Also, I feel like the error messages could be improved more, but that can be for a later patch.)

@ailzhang
Copy link
Contributor

cc: @li-roy

@fmassa
Copy link
Member

fmassa commented Aug 28, 2018

ping @li-roy about Ed's comment on tests.

@zou3519
Copy link
Contributor

zou3519 commented Sep 10, 2018

This is pretty hi-pri. I'll give it a review after you update it @li-roy

Related: #11427

@li-roy
Copy link
Contributor Author

li-roy commented Sep 11, 2018

@pytorchbot retest this please

@@ -7095,6 +7095,10 @@ def test_parsing_intlist(self):
self.assertRaises(TypeError, lambda: torch.ones(np.array(3, 3)))
self.assertRaises(TypeError, lambda: torch.ones((np.array(3, 3))))

# fail parse with additional positional args after intlist arg
self.assertRaises(TypeError, lambda: torch.LongTensor((6, 0), 1, 1, 0))

This comment was marked as off-topic.

Copy link
Contributor

@zou3519 zou3519 left a comment

Choose a reason for hiding this comment

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

LGTM. Let me know what the error message looks like though (a self.assertRaisesRegex would be great)

@li-roy
Copy link
Contributor Author

li-roy commented Sep 13, 2018

@pytorchbot retest this please

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

li-roy has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strange legacy constructor parsing behavior
6 participants