Aten roi align #209
Aten roi align #209
Conversation
…must also equal K. Converter still does not work for aligned=True
…ap which demonstrates corrupted input.
…t output height and width (original tests were symmetrical). Still failing some tests
| # batch_indices = batch_indices2 | ||
| elif boxes_ref.shape().lens()[1] == 4: | ||
| # batch_indices3=range(boxes_ref.shape().lens()[0]) | ||
| # boxes2 = boxes_ref |
There was a problem hiding this comment.
Clean up comments please, this just looks like unused debug code?
| elif boxes_ref.shape().lens()[1] == 4: | ||
| # batch_indices3=range(boxes_ref.shape().lens()[0]) | ||
| # boxes2 = boxes_ref | ||
| # This isn't supported because torchvision roi_align.default() doesn't support it |
There was a problem hiding this comment.
I think its not supported because the list inputs was giving issues? I'd suggest leaving a TODO comment here to try and support in the future
There was a problem hiding this comment.
The torchvision module threw errors when it was called with that shape of input in the aten test. The only way I see to add support is to avoid testing it against the torchvision base. Other than that, it should work fine with Line 235 commented in.
There was a problem hiding this comment.
I am not sure what you mean, this is the case where the rois are a list of tensors right? Thats not even supported by the aten op. This is only for the acc op case where the rois are defined as a list
On a second look, that if statement does not work at all, if the rois are defined as a list, your boxes_ref variable wont be a migraphx instruction and the .shape call isnt valid anyways
| @@ -21,4 +21,4 @@ cd AMDMIGraphX | |||
| rbuild build -d depend -DBUILD_TESTING=Off -DCMAKE_INSTALL_PREFIX=/opt/rocm/ --cxx=/opt/rocm/llvm/bin/clang++ -DGPU_TARGETS=$GPU_ARCH | |||
|
|
|||
| cd build | |||
| make install | |||
| make -j88 install | |||
There was a problem hiding this comment.
Is there a reason we don't want to use the multithread flag?
There was a problem hiding this comment.
Well usually the number you set depends on the system. Also, the rbuild command does the building part here (which i think is multithreaded?), the make install just copies the files over to opt/rocm and so multithreading that doesnt much of a difference
| @@ -100,7 +100,6 @@ def _initialize(self): | |||
| if not self.program.is_compiled(): | |||
| if self.quantize_fp16: | |||
| migraphx.quantize_fp16(self.program) | |||
|
|
|||
| # batch_indices3=range(boxes_ref.shape().lens()[0]) | ||
| # boxes2 = boxes_ref | ||
| # This isn't supported at this time because torchvision roi_align.default() doesn't support it |
There was a problem hiding this comment.
Clean up debug comments. Also can we change the isnt supported comment to a TODO comment. Its not supported at this time because we are having trouble consuming a list input in the test cases atm. "torchvision roi_align.default() doesn't support it" is not a valid reason not to support it in the acc converter.
| @@ -33,6 +33,7 @@ | |||
|
|
|||
| import torch | |||
| from typing import cast, Iterable, List, Sequence | |||
| import torchvision | |||
There was a problem hiding this comment.
| import torchvision | |
| try: | |
| import torchvision | |
| except ImportError: | |
| pass |
This is not a mandatory prerequisite for torch_migraphx so wrap it in a try/except
| @@ -373,6 +374,27 @@ def clamp(*, input, min=None, max=None): | |||
| return torch.clamp(input=input, min=min, max=max) | |||
|
|
|||
|
|
|||
| @register_acc_op_mapping( | |||
There was a problem hiding this comment.
| @register_acc_op_mapping( | |
| if 'torchvision' in sys.modules: | |
| @register_acc_op_mapping( |
wrap the roi_align function definition in this if so that it doesnt blow up if torchvision isnt installed
| ]), | ||
| [7, 6])] | ||
| ) | ||
| def test_roialign(input, boxes, output_size, spatial_scale, sampling_ratio, aligned): |
There was a problem hiding this comment.
can you move this to test_torchvision_models_fx.py and follow the example there to skip if torchvision is not installed
| [7, 6]) | ||
| ] | ||
| ) | ||
| def test_roialign(op_alias, input, boxes, output_size, spatial_scale, sampling_ratio, aligned): |
There was a problem hiding this comment.
Similar to the fx test comment, this should be in a test_torchvision_models_dynamo.py file, and it should mark to skip in the same way if torchvision is not installed.
Add aten and acc converters for the RoiAlign op.
This change is dependent on Migraphx PR 3482. It can't be tested or merged until that branch is merged. (Test locally using MigraphX branch
roialign_fixAs of date of PR creation, the aten converter test
test_roialign_dis still incomplete.