-
Notifications
You must be signed in to change notification settings - Fork 97
Integrate numba into setuptools [ESD-1156] #687
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
Conversation
9edaa98 to
0e3b31d
Compare
|
CFFI initial concept OK. Numba CC compiler getting stuck in |
|
|
Not reproducible in standalone compilation, only one unrelated warning: |
|
Seems to be somehow related to Python version.
On 3.5.0 build does not hang and it uses |
|
numba.pycc issue with py27 replicates with the example code from https://numba.pydata.org/numba-doc/dev/user/pycc.html |
|
Replicates on OSX with |
|
To record the current status:
from numba.pycc import CC
cc = CC('my_module')
# Uncomment the following line to print out the compilation steps
cc.verbose = True
@cc.export('multf', 'f8(f8, f8)')
@cc.export('multi', 'i4(i4, i4)')
def mult(a, b):
return a * b
@cc.export('square', 'f8(f8)')
def square(a):
return a ** 2
cc.compile()py35 success py27 hangs After narrowing down the scope with added debug prints in |
|
Appears that build uses Code is identical in both py27 and py35 numpy installations. After forcing serial build, build doesn't hang. |
|
Narrowed down to |
|
Monkeypatching to serial build only helps. Needs to be investigated how rugged solution this is. from numba.pycc import CC
from distutils.ccompiler import CCompiler
from numpy.distutils.misc_util import get_num_build_jobs
from numpy.distutils.ccompiler import _global_lock, _job_semaphore, _processing_files
from numpy.distutils import log
import sys
import types
def replace_method(klass, method_name, func):
if sys.version_info[0] < 3:
m = types.MethodType(func, None, klass)
setattr(klass, method_name, m)
def CCompiler_compile(self, sources, output_dir=None, macros=None,
include_dirs=None, debug=0, extra_preargs=None,
extra_postargs=None, depends=None):
"""
Compile one or more source files.
Please refer to the Python distutils API reference for more details.
Parameters
----------
sources : list of str
A list of filenames
output_dir : str, optional
Path to the output directory.
macros : list of tuples
A list of macro definitions.
include_dirs : list of str, optional
The directories to add to the default include file search path for
this compilation only.
debug : bool, optional
Whether or not to output debug symbols in or alongside the object
file(s).
extra_preargs, extra_postargs : ?
Extra pre- and post-arguments.
depends : list of str, optional
A list of file names that all targets depend on.
Returns
-------
objects : list of str
A list of object file names, one per source file `sources`.
Raises
------
CompileError
If compilation fails.
"""
# This method is effective only with Python >=2.3 distutils.
# Any changes here should be applied also to fcompiler.compile
# method to support pre Python 2.3 distutils.
global _job_semaphore
jobs = get_num_build_jobs()
# setup semaphore to not exceed number of compile jobs when parallelized at
# extension level (python >= 3.5)
with _global_lock:
if _job_semaphore is None:
_job_semaphore = threading.Semaphore(jobs)
if not sources:
return []
ccomp = self.compiler_so
display = "C compiler: %s\n" % (' '.join(ccomp),)
log.info(display)
macros, objects, extra_postargs, pp_opts, build = \
self._setup_compile(output_dir, macros, include_dirs, sources,
depends, extra_postargs)
cc_args = self._get_cc_args(pp_opts, debug, extra_preargs)
display = "compile options: '%s'" % (' '.join(cc_args))
if extra_postargs:
display += "\nextra options: '%s'" % (' '.join(extra_postargs))
log.info(display)
def single_compile(args):
obj, (src, ext) = args
# check if we are currently already processing the same object
# happens when using the same source in multiple extensions
while True:
# need explicit lock as there is no atomic check and add with GIL
with _global_lock:
# file not being worked on, start working
if obj not in _processing_files:
_processing_files.add(obj)
break
# wait for the processing to end
time.sleep(0.1)
try:
# retrieve slot from our #job semaphore and build
with _job_semaphore:
self._compile(obj, src, ext, cc_args, extra_postargs, pp_opts)
finally:
# register being done processing
with _global_lock:
_processing_files.remove(obj)
for o in build.items():
single_compile(o)
# Return *all* object filenames, not just the ones we just built.
return objects
replace_method(CCompiler, 'compile', CCompiler_compile)
cc = CC('my_module')
# Uncomment the following line to print out the compilation steps
cc.verbose = True
@cc.export('multf', 'f8(f8, f8)')
@cc.export('multi', 'i4(i4, i4)')
def mult(a, b):
return a * b
@cc.export('square', 'f8(f8)')
def square(a):
return a ** 2
cc.compile() |
6738e80 to
eeba8e3
Compare
|
Next problem. |
|
Actual integration to distutils in case of numba needs to be investigated. Main problem being that libsbp uses actually |
eeba8e3 to
4fe36a1
Compare
|
Next some problem with the benchmark script: |
a8db61d to
e15ec3f
Compare
e15ec3f to
e859e93
Compare
silverjam
left a comment
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 great! Just a few nit and questions.
| INSTALL_REQUIRES += [i.strip() for i in f.readlines()] | ||
|
|
||
| with open(os.path.join(filedir, 'setup_requirements.txt')) as f: | ||
| INSTALL_REQUIRES += [i.strip() for i in f.readlines()] |
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.
Do we want setup_requirements.txt in INSTALL_REQUIRES? Does this make numba and friends a library dependency? Is there an equivalent "build requirements"?
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.
I need to revisit this. I wasn't feeling I was quite done with the thought process with this. It's complicated to make a clean solution that caters for all the use cases.
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.
I think the main points are close to:
- with
sdistpackage these requirements are needed as you are buildingsbpand theparse_jitmodule from the sources - with
bdist_wheelpackage these requirements are not needed but the wheel needs to be built and deployed for each Python version separately
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.
sdist is not uploaded to PyPi so I'll just add the version specific wheels and remove the setup_requirements from the package.
python/sbp/constants.py
Outdated
| # device. | ||
| SENDER_ID = 0x42 | ||
|
|
||
| _crc16_tab = [ |
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.
| _crc16_tab = [ | |
| crc16_tab = [ |
python/sbp/jit/parse.py
Outdated
|
|
||
| from sbp.constants import SENDER_ID as _SENDER_ID | ||
| from sbp.constants import SBP_PREAMBLE as _SBP_PREAMBLE | ||
| from sbp.constants import _crc16_tab |
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.
| from sbp.constants import _crc16_tab | |
| crc16_tab = [ |
python/sbp/jit/parse.py
Outdated
| cc = CC(module_name) | ||
| cc.verbose = True | ||
|
|
||
| crc16_tab = np.array(_crc16_tab, dtype=np.uint16) |
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.
| crc16_tab = np.array(_crc16_tab, dtype=np.uint16) | |
| np_crc16_tab = np.array(_crc16_tab, dtype=np.uint16) |
python/sbp/jit/parse.py
Outdated
| """CRC16 implementation acording to CCITT standards.""" | ||
| for index in range(offset, offset + length): | ||
| data = buf[index] | ||
| lookup = crc16_tab[((nb.u2(crc) >> 8) & nb.u2(0xFF)) ^ (data & nb.u2(0xFF))] |
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.
| lookup = crc16_tab[((nb.u2(crc) >> 8) & nb.u2(0xFF)) ^ (data & nb.u2(0xFF))] | |
| lookup = np_crc16_tab[((nb.u2(crc) >> 8) & nb.u2(0xFF)) ^ (data & nb.u2(0xFF))] |
python/sbp/msg.py
Outdated
|
|
||
| from sbp.constants import SENDER_ID as _SENDER_ID | ||
| from sbp.constants import SBP_PREAMBLE as _SBP_PREAMBLE | ||
| from sbp.constants import _crc16_tab |
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.
| from sbp.constants import _crc16_tab | |
| from sbp.constants import crc16_tab |
python/sbp/msg.py
Outdated
| parse_jit = importlib.import_module('sbp.jit.' + parse_jit_name) | ||
|
|
||
|
|
||
| crc16_tab = np.array(_crc16_tab, dtype=np.uint16) |
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.
| crc16_tab = np.array(_crc16_tab, dtype=np.uint16) | |
| np_crc16_tab = np.array(crc16_tab, dtype=np.uint16) |
python/sbp/msg.py
Outdated
| """CRC16 implementation acording to CCITT standards.""" | ||
| for ch in bytearray(s): # bytearray's elements are integers in both python 2 and 3 | ||
| crc = ((crc << 8) & 0xFFFF) ^ _crc16_tab[((crc >> 8) & 0xFF) ^ (ch & 0xFF)] | ||
| crc = ((crc << 8) & 0xFFFF) ^ crc16_tab[((crc >> 8) & 0xFF) ^ (ch & 0xFF)] |
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.
| crc = ((crc << 8) & 0xFFFF) ^ crc16_tab[((crc >> 8) & 0xFF) ^ (ch & 0xFF)] | |
| crc = ((crc << 8) & 0xFFFF) ^ np_crc16_tab[((crc >> 8) & 0xFF) ^ (ch & 0xFF)] |
python/sbp/jit/parse.py
Outdated
| include_dirs=None, debug=0, extra_preargs=None, | ||
| extra_postargs=None, depends=None): | ||
| """ | ||
| Compile one or more source files. |
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.
I think we can delete their docs, and maybe just insert a link to the original source?
Numba code is built when the module is called for the first time. This way the code will be optimized for the processor at hand instead of more generic prebuild package. In following import calls build step is not needed. This means that numba is still a requirement.
c7628b9 to
1e09b98
Compare
|
@silverjam to review the latest changes and decide on the merging. |
|
Changes look good but I can't figure out how to build on Windows, will try to spend a few minutes working this out, otherwise we'll need to revisit later. |
We need to work around a permission denied error on Windows.
| - sudo apt-get -y -o Dpkg::Options::="--force-confnew" install docker-ce | ||
| - sudo pip install tox | ||
| - git clone -b master https://github.com/swift-nav/piksi_tools.git ../piksi_tools | ||
| - git clone -b pmiettinen/esd-1156-numba-deployment https://github.com/swift-nav/piksi_tools.git ../piksi_tools |
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.
fix after merge
https://swift-nav.atlassian.net/projects/ESD/issues/ESD-1156
Depends swift-nav/piksi_tools#1050