Skip to content

Avoid dynamic dispatch inside the omp loop in AdaptiveAvgPool2d#20366

Closed
crcrpar wants to merge 3 commits intopytorch:masterfrom
crcrpar:improve-adaptiveAvgPool2d
Closed

Avoid dynamic dispatch inside the omp loop in AdaptiveAvgPool2d#20366
crcrpar wants to merge 3 commits intopytorch:masterfrom
crcrpar:improve-adaptiveAvgPool2d

Conversation

@crcrpar
Copy link
Collaborator

@crcrpar crcrpar commented May 10, 2019

This PR changes CPU implementation of AdaptiveAveragePool2D by

  • move dispatch to outside the OpenMP loop
  • support fp16

@zou3519
Copy link
Contributor

zou3519 commented May 10, 2019

@crcrpar tests are failing:

May 10 16:11:22 ======================================================================
May 10 16:11:22 FAIL: test_nn_adaptive_avg_pool2d (__main__.TestJitGeneratedFunctional)
May 10 16:11:22 ----------------------------------------------------------------------
May 10 16:11:22 Traceback (most recent call last):
May 10 16:11:22   File "test_jit.py", line 14058, in wrapper
May 10 16:11:22     return fn(*args, **kwargs)
May 10 16:11:22   File "test_jit.py", line 14108, in do_test
May 10 16:11:22     run_test()
May 10 16:11:22   File "test_jit.py", line 14100, in run_test
May 10 16:11:22     check_against_reference(self, script_fn, fn, f_args_variable, kwargs_variable, no_grad=no_grad)
May 10 16:11:22   File "test_jit.py", line 13357, in check_against_reference
May 10 16:11:22     self.assertEqual(grads, grads_test)
May 10 16:11:22   File "/var/lib/jenkins/workspace/test/common_utils.py", line 498, in assertEqual
May 10 16:11:22     self.assertEqual(x_, y_, prec, message)
May 10 16:11:22   File "/var/lib/jenkins/workspace/test/common_utils.py", line 483, in assertEqual
May 10 16:11:22     assertTensorsEqual(x, y)
May 10 16:11:22   File "/var/lib/jenkins/workspace/test/common_utils.py", line 475, in assertTensorsEqual
May 10 16:11:22     self.assertLessEqual(max_err, prec, message)
May 10 16:11:22 AssertionError: tensor(2.) not less than or equal to 1e-05 :

@crcrpar
Copy link
Collaborator Author

crcrpar commented May 13, 2019

@zou3519 Sorry, I forgot the existence of JIT tests.

@jeffreyksmithjr jeffreyksmithjr added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 13, 2019
ezyang
ezyang previously requested changes May 17, 2019
View reviewed changes
Copy link
Contributor

@ezyang ezyang left a comment

Choose a reason for hiding this comment

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

CI is failing

@crcrpar
Copy link
Collaborator Author

crcrpar commented May 18, 2019

I had a bit busy weekdays and am sorry for being late.

I think I fixed the problem and the asan test failure seems unrelated to this because it occurred in DataLoader test as follows.

...
May 18 02:17:46 test_proper_exit (__main__.TestDataLoader)
May 18 02:17:51 There might be ConnectionResetError or leaked semaphore warning (due to dirty process exit), but they are all safe to ignore ... Process ErrorTrackingProcess-17:
May 18 02:17:51 Traceback (most recent call last):
May 18 02:17:51   File "/opt/conda/lib/python3.6/multiprocessing/process.py", line 258, in _bootstrap
May 18 02:17:51     self.run()
May 18 02:17:51   File "/var/lib/jenkins/workspace/test/test_dataloader.py", line 227, in run
May 18 02:17:51     super(ErrorTrackingProcess, self).run()
May 18 02:17:51   File "/opt/conda/lib/python3.6/multiprocessing/process.py", line 93, in run
May 18 02:17:51     self._target(*self._args, **self._kwargs)
May 18 02:17:51   File "/var/lib/jenkins/workspace/test/test_dataloader.py", line 439, in _test_proper_exit
May 18 02:17:51     raise RuntimeError('Loader error')
May 18 02:17:51 RuntimeError: Loader error
May 18 02:17:58 /opt/conda/lib/python3.6/multiprocessing/semaphore_tracker.py:143: UserWarning: semaphore_tracker: There appear to be 14 leaked semaphores to clean up at shutdown
May 18 02:17:58   len(cache))
May 18 02:18:01 Process ErrorTrackingProcess-19:
...

@crcrpar
Copy link
Collaborator Author

crcrpar commented May 22, 2019

@zou3519 @ezyang Could you check this?

@crcrpar
Copy link
Collaborator Author

crcrpar commented May 22, 2019

I found https://github.com/pytorch/pytorch/wiki/TH-to-ATen-porting-guide#dispatch-and-openmp is a bit outdated because now ATen has parallel_for function.

@ezyang ezyang self-requested a review May 22, 2019 19:17
@ezyang
Copy link
Contributor

ezyang commented May 22, 2019

Quick tip: you have to "dismiss" the review for it to show up in my review inbox

@ezyang
Copy link
Contributor

ezyang commented May 22, 2019

I think the problem discussed in the porting guide is obsolete now, because lambdas work inside lambdas. So you don't need to make separate functions anymore.

@ezyang
Copy link
Contributor

ezyang commented May 22, 2019

But I won't tell you to fix it! No problem with more functions.

facebook-github-bot
facebook-github-bot reviewed May 22, 2019
View reviewed changes
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.

@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

zdevito pushed a commit to zdevito/ATen that referenced this pull request May 23, 2019
@crcrpar @facebook-github-bot
Avoid dynamic dispatch inside the omp loop in AdaptiveAvgPool2d (#20366)
343b055 
Summary:
This PR changes CPU implementation of `AdaptiveAveragePool2D` by
- move dispatch to outside the OpenMP loop
- support fp16
Pull Request resolved: pytorch/pytorch#20366

Differential Revision: D15456069

Pulled By: ezyang

fbshipit-source-id: 00fa2916f8b136af9f5c8b5db0eca4619f9f5bac
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented May 24, 2019

@ezyang merged this pull request in 48424a6.

@crcrpar
Copy link
Collaborator Author

crcrpar commented May 24, 2019

OK, thank you very much.

@crcrpar crcrpar deleted the improve-adaptiveAvgPool2d branch May 24, 2019 03:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@ezyang ezyang ezyang approved these changes

@gchanan gchanan Awaiting requested review from gchanan

@zou3519 zou3519 Awaiting requested review from zou3519

Assignees

No one assigned

Labels

Merged open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@crcrpar @zou3519 @ezyang @facebook-github-bot @jeffreyksmithjr @pytorchbot @mruberry