Skip to content

Conversation

@KsenijaS
Copy link
Contributor

@KsenijaS KsenijaS commented Aug 12, 2020

During scripting, combination of shape (or size()) and slice (e.g x.shape[2:]) produces following error:
slice() missing 1 required positional argument: 'step'
This happens because aten::slice has 2 signatures:

  • aten::slice(Tensor self, int dim, int start, int end, int step) -> Tensor
  • aten::slice(t[] l, int start, int end, int step) -> t[]

and when a list is passed instead of tensor the 2nd of the two slice signatures is called, and since it has 4 instead of 5 arguments it produces the above exception.

@dr-ci
Copy link

dr-ci bot commented Aug 12, 2020

💊 CI failures summary and remediations

As of commit a04d017 (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 26 times.

@zhangguanheng66 zhangguanheng66 added module: onnx Related to torch.onnx triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Aug 12, 2020
Copy link
Contributor

@neginraoof neginraoof left a comment

Choose a reason for hiding this comment

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

Left some comments. Thanks.

@neginraoof
Copy link
Contributor

Thanks. Can you add more info about the new slice signature you're adding as part of the PR description?

@neginraoof
Copy link
Contributor

@KsenijaS test_slice is failing. Please take a look.

Copy link
Contributor

@neginraoof neginraoof left a comment

Choose a reason for hiding this comment

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

Looks good.

@KsenijaS
Copy link
Contributor Author

@bzinodev can we please merge this? Thanks!

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.

@bzinodev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@KsenijaS
Copy link
Contributor Author

@bzinodev I see there are some internal Facebook tests failing? Can you share what the failures are? Can we merge athis PR despite those failures?

@facebook-github-bot
Copy link
Contributor

@bzinodev merged this pull request in 820c4b0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merged module: onnx Related to torch.onnx open source 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.

8 participants