fix crash when profiler not enabled #10306
Conversation
| @@ -398,7 +398,8 @@ class ThreadedEngine : public Engine { | |||
| } | |||
|
|
|||
| int bulk_size() const override { | |||
| return profiler::Profiler::Get()->AggregateRunning() ? 0 : BulkStatusStore::Get()->bulk_size; | |||
| const profiler::Profiler *prof = profiler::Profiler::Get(); | |||
There was a problem hiding this comment.
why would profiler::get return nullptr when it's not enabled?
There was a problem hiding this comment.
I didn't think it did either. When profiler isn't enabled in the build at all.
The ability to turn profiler off in the build will be removed soon
|
@marcoabreu any idea why CI wasn't able to catch this? |
|
Is there a build that doesn't turn on profiler? |
|
I think I'll just remove the ability to turn off profiler at build time tomorrow |
|
Here is removing profiler option: #10308 |
|
everyone ok for this to merge when it completes? |
|
I'm ok for merging. |
|
@piiswrong I think all builds in CI are turning on profiler by default |
|
attempt number 6 to build with CI... |
| @@ -1784,3 +1784,4 @@ def test_kernel_error_checking(): | |||
| if __name__ == '__main__': | |||
| import nose | |||
| nose.runmodule() | |||
| mx.base._notify_shutdown() | |||
There was a problem hiding this comment.
Wouldn't this call problems if another test is run afterwards?
|
I know the notify_shutdown() is only an attempt to fix the hanging gpu test, but could you make sure to remove it before you merge this PR? |
|
1) Nope
2) It was just an experimental change to see if it helped, so it will be
reverted
…On Thu, Mar 29, 2018 at 2:45 PM, Marco de Abreu ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In tests/python/gpu/test_operator_gpu.py
<#10306 (comment)>
:
> @@ -1784,3 +1784,4 @@ def test_kernel_error_checking():
if __name__ == '__main__':
import nose
nose.runmodule()
+ mx.base._notify_shutdown()
Wouldn't this call problems if a test is run afterwards?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#10306 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKts_WX6JmtaQpgA73VHEQzUTPqCwOt3ks5tjVX1gaJpZM4S_d6M>
.
|
* fix crash when profiler not enabled * fix * Update graph_executor.cc * Update graph_executor.cc * use nosetests to try and prevent thehang * shutdown after GPU pass * remove temp * remove temp
* fix crash when profiler not enabled * fix * Update graph_executor.cc * Update graph_executor.cc * use nosetests to try and prevent thehang * shutdown after GPU pass * remove temp * remove temp
* fix crash when profiler not enabled * fix * Update graph_executor.cc * Update graph_executor.cc * use nosetests to try and prevent thehang * shutdown after GPU pass * remove temp * remove temp
* fix crash when profiler not enabled * fix * Update graph_executor.cc * Update graph_executor.cc * use nosetests to try and prevent thehang * shutdown after GPU pass * remove temp * remove temp
Description
(Brief description on what this PR is about)
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments