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

[ONNX] Fix for constant folding: Slice, Added ReduceL1 and ReduceL2 #35280

Closed
wants to merge 20 commits into from

Conversation

@neginraoof
Copy link
Collaborator

@neginraoof neginraoof commented Mar 24, 2020

1- Added support for constant folding onnx::ReduceL1 and onnx::ReduceL2
2- Fixed constant folding for slice as onnx::Slice opset 11 supports negative axes and indices
3- Updated export of select opset 11
4- Separated test environment for test_utility_functions as environment variables could be overwritten by caffe2 quantization tests on CI

@dr-ci
Copy link

@dr-ci dr-ci bot commented Mar 24, 2020

💊 CircleCI build failures summary and remediations

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


  • 1/1 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

The following build failures do not appear to be due to upstream breakages (reran 1 job to discount flakiness):

See CircleCI build pytorch_windows_vs2019_py36_cuda10.1_test1 (1/1)

Step: "Test" (full log | pattern match details) <confirmed not flaky by 2 failures>

pip._vendor.urllib3.exceptions.ProtocolError: ("Connection broken: ConnectionResetError(10054, 'An existing connection was forcibly closed by the remote host', None, 10054, None)", ConnectionResetError(10054, 'An existing connection was forcibly closed by the remote host', None, 10054, None))
  File "C:\Jenkins\Miniconda3\lib\site-packages\pip\_internal\network\utils.py", line 39, in response_chunks 
    decode_content=False, 
  File "C:\Jenkins\Miniconda3\lib\site-packages\pip\_vendor\urllib3\response.py", line 564, in stream 
    data = self.read(amt=amt, decode_content=decode_content) 
  File "C:\Jenkins\Miniconda3\lib\site-packages\pip\_vendor\urllib3\response.py", line 529, in read 
    raise IncompleteRead(self._fp_bytes_read, self.length_remaining) 
  File "C:\Jenkins\Miniconda3\lib\contextlib.py", line 99, in __exit__ 
    self.gen.throw(type, value, traceback) 
  File "C:\Jenkins\Miniconda3\lib\site-packages\pip\_vendor\urllib3\response.py", line 443, in _error_catcher 
    raise ProtocolError("Connection broken: %r" % e, e) 
pip._vendor.urllib3.exceptions.ProtocolError: ("Connection broken: ConnectionResetError(10054, 'An existing connection was forcibly closed by the remote host', None, 10054, None)", ConnectionResetError(10054, 'An existing connection was forcibly closed by the remote host', None, 10054, None)) 
 
(base) circleci@PACKER-5E70C982 C:\Users\circleci\project>if "10" == "9" goto cuda_build_9  
 
(base) circleci@PACKER-5E70C982 C:\Users\circleci\project>if "10" == "10" goto cuda_build_10  
 
(base) circleci@PACKER-5E70C982 C:\Users\circleci\project>pushd .  
 
(base) circleci@PACKER-5E70C982 C:\Users\circleci\project>if "" == "" (call "C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Auxiliary\Build\vcvarsall.bat" x64 )  else (call "C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Auxiliary\Build\vcvarsall.bat" x64 -vcvars_ver= )  
********************************************************************** 
** Visual Studio 2019 Developer Command Prompt v16.5.0 

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.

See how this bot performed.

This comment has been revised 38 times.

Loading

@albanD albanD requested a review from houseroad Mar 24, 2020
@albanD albanD added the triaged label Mar 24, 2020
Copy link
Contributor

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

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

Loading

torch/csrc/jit/passes/onnx/constant_fold.cpp Outdated Show resolved Hide resolved
Loading
torch/csrc/jit/passes/onnx/constant_fold.cpp Show resolved Hide resolved
Loading
torch/onnx/symbolic_opset11.py Show resolved Hide resolved
Loading
// It needs to be adjusted (+= dim value) for aten op
auto less_mask = at::lt(indices, 0);
auto indices_corr = at::add(indices, inputTensorValues[0].sizes()[axis]);
auto indices_masked = at::where(less_mask, indices_corr, indices);
Copy link
Contributor

@lara-hdr lara-hdr Mar 24, 2020

Choose a reason for hiding this comment

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

I am not sure to understand what is happening here.
My understanding is that :

  • indices contains positive and negative indices
  • less mask is a mask to identify the negative indices
  • indices_corr is the "corrected" indices; inputTensorValues[0].sizes()[axis] is added for all indices even if they are positive ?
  • indices_masked is a mask on the corrected negative indices ?

Loading

Copy link
Collaborator Author

@neginraoof neginraoof Mar 25, 2020

Choose a reason for hiding this comment

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

The "where" op, takes values from "indices_corr" where condition (indices < 0) is true, and takes values from "indices" where condition is false. So in cases where indices < 0, we have indices += dim value.

Loading

Copy link
Contributor

@lara-hdr lara-hdr Mar 26, 2020

Choose a reason for hiding this comment

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

do we have a test covering this pass?

Loading

Copy link
Collaborator Author

@neginraoof neginraoof Mar 27, 2020

Choose a reason for hiding this comment

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

Yes, this is in test_utility_functions line 157. Index is < 0

Loading

neginraoof added 2 commits Mar 30, 2020
… into neraoof/constFoldNorm

# Conflicts:
#	torch/csrc/jit/passes/onnx/constant_fold.cpp
Copy link
Contributor

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

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

Loading

@neginraoof
Copy link
Collaborator Author

@neginraoof neginraoof commented Apr 1, 2020

@houseroad Can we close this? Thanks

Loading

Copy link
Member

@houseroad houseroad left a comment

Thanks

Loading

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented Apr 1, 2020

@houseroad merged this pull request in 60a3e82.

Loading

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

Successfully merging this pull request may close these issues.

None yet

7 participants