-
Notifications
You must be signed in to change notification settings - Fork 13
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
fixes-459 #460
fixes-459 #460
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one log message to fix, otherwise looks good to me.
cubical/workers.py
Outdated
return nthread | ||
except: | ||
numba.config.THREADING_LAYER = "default" | ||
print("Cannot use TDD threading (check your installation). Dropping the number of solver threads to 1", file=log(0, "red")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be TBB (thread building blocks).
Yup. As discussed -:- https://numba.pydata.org/numba-doc/latest/user/threading-layer.html This used to work with earlier versions of numba though so I'm not sure what has changed to make things execute "unsafely". Perhaps they switched over their default threaded model. |
Just putting my two cents here. Note these issues on Numba: numba/numba#6108 and numba/numba#7148. It seems that it has something to do with discovery of the |
Ok I tried exporting the LD_LIBRARY_PATH (technically it should not be needed when the virtualenv is activated expressly though) Numba -s reports successful import
I do have
However, when I try to execute the basic function def set_numba_threading(nthread):
try:
numba.config.THREADING_LAYER = "safe"
@numba.njit(parallel=True)
def foo(a, b):
return a + b
foo(np.arange(5), np.arange(5))
return nthread
except:
numba.config.THREADING_LAYER = "default"
print("Cannot use TBB threading (check your installation). Dropping the number of solver threads to 1", file=log(0, "red"))
return 1 I get
So it is still picking up the older system libraries version. Unfortunately I can't uninstall that version --- it will break several system packages |
Ok hang on... found another place where OMP is being used -- the degridder.... J It may run into a screwup when forks and threads are used. I suggest we switch to workqueue if TBB fails to load then on top of that set the environment variables accordingly -- if workers.py sets nthread >1 then the degridder needs to go to OMP_NUM_THREADS == 1 |
I'm not sure how this did not give issues before. My best guess is numpy now invokes OMP before we fork and then it becomes unsafe to use..... |
Nope workqueue don't completely solve the issue -- things still go pot if threads > 1 is used on workqueue -- and it is not a memory issue. I'm testing this on com08 Edit : at least on 18.04 it looks like one would need to compile packages at system level (possibly just to include headers for TBB ???) I have no idea how to get things working in a venv. I've traced it down to stem from the numba code with these changes |
Alright I'm happy this now works as advertised -- can't believe nobody picked up the issue with the previously used small angle approximation for getting the ra dec of the facet center for beam application:
original convolved model flux (subject to a slightly different beam evaluation due to the regular facets used in DDF - so a small difference to be expected
Apparent peak convolved flux of the source:
@viralp please take note -- you have previously tried using the beam within cubical and not getting decent subtraction when peeling sources from intrinsic models The example use is (pointing center may be set to be the phase center if you did not mosaic via 'DataPhaseDir':
This work is done in preparation for SKA-MID. I will next port heterogeneous beams to this package |
@JSKenyon please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me, bar my single comment.
return a + b | ||
foo(np.arange(5), np.arange(5)) | ||
return nthread | ||
except: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unqualified excepts are generally frowned upon. Does this not raise a consistent exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it raises a massively complex numba exception which I'm not sure how to properly catch. let me see if I can reproduce. I'm just worried this exception interface will change (numba seems constantly evolving)
This fixes #459.
As near as I can tell the issue is in the use of threads inside numba..... TDD fails to import in the new numba (even if I follow their instructions and install from pip.... @JSKenyon)
I still can't track down this super annoying error -- that part of the code is a bit cryptic maybe @o-smirnov can be of some assistance here
At least it runs through again now
Running with