-
Notifications
You must be signed in to change notification settings - Fork 10
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
Update Numba to 0.57 #538
base: main
Are you sure you want to change the base?
Update Numba to 0.57 #538
Conversation
f027248
to
1e31af4
Compare
|
5078638
to
50fc2f2
Compare
Blocked on HeavyDB 7.0 release on conda-forge as Numba 0.57 uses LLVM 14. |
rbc/irtools.py
Outdated
# check LLVM version before compiling to LLVM | ||
server_llvm_version = target_info.llvm_version | ||
client_llvm_version = llvm.llvm_version_info | ||
|
||
if (server_llvm_version[0], client_llvm_version[0]) == (11, 14): | ||
c_llvm = '.'.join(map(str, client_llvm_version)) | ||
s_llvm = '.'.join(map(str, server_llvm_version)) | ||
flag = 'RBC_DISABLE_LLVM_MISMATCH_ERROR' | ||
msg = (f'The client LLVM version ({c_llvm}) is greater than the server ' | ||
f'LLVM version ({s_llvm}). This is known to be unsupported. ' | ||
'Please, downgrade to a previous release of Numba that uses the ' | ||
'same LLVM version as the HeavyDB server. For more information, ' | ||
'see the table below:\n\n' | ||
'https://github.com/numba/llvmlite#compatibility\n' | ||
'https://github.com/heavyai/heavydb#dependencies\n\n' | ||
f'To disable this error, run RBC with {flag}=1 flag enabled.') | ||
|
||
DISABLE_LLVM_MISMATCH_ERROR = int(os.environ.get(flag, False)) | ||
if not DISABLE_LLVM_MISMATCH_ERROR: | ||
raise LLVMVersionMismatchError(msg) |
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.
Would this be the most appropriated place to put this check?
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.
Could you create a utility function that processes the llvm version mismatches?
I am not sure if raising an exception is an appropriate measure for avoiding possible problems from llvm version mismatches. We have had these mismatches in the past all the time and typically the mismatch of versions is not a problem except for certain use cases. The LLVM 11 vs llvmlite LLVM 14 is the first case where the problem is fatal for most cases (because llvm 14 generates LLVM IR containing features (attributes) that llvm 11 parser does not know how to handle, and even here we can implement a workaround). Here, in the case of 11 vs 14, raising an exception from rbc side is reasonable indeed but for the other cases, I think submitting a warning message with detailed instructions should be sufficient.
The llvm version check should be of the only-once kind and it could be carried out when rbc establishes the connection to the heavydb server. Checking it every time in compile_to_LLVM
would be too much indeed.
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.
Okay, I can change that. We only check for the LLVM 11/14 combination, which is known to be problematic.
rbc/irtools.py
Outdated
# check LLVM version before compiling to LLVM | ||
server_llvm_version = target_info.llvm_version | ||
client_llvm_version = llvm.llvm_version_info | ||
|
||
if (server_llvm_version[0], client_llvm_version[0]) == (11, 14): | ||
c_llvm = '.'.join(map(str, client_llvm_version)) | ||
s_llvm = '.'.join(map(str, server_llvm_version)) | ||
flag = 'RBC_DISABLE_LLVM_MISMATCH_ERROR' | ||
msg = (f'The client LLVM version ({c_llvm}) is greater than the server ' | ||
f'LLVM version ({s_llvm}). This is known to be unsupported. ' | ||
'Please, downgrade to a previous release of Numba that uses the ' | ||
'same LLVM version as the HeavyDB server. For more information, ' | ||
'see the table below:\n\n' | ||
'https://github.com/numba/llvmlite#compatibility\n' | ||
'https://github.com/heavyai/heavydb#dependencies\n\n' | ||
f'To disable this error, run RBC with {flag}=1 flag enabled.') | ||
|
||
DISABLE_LLVM_MISMATCH_ERROR = int(os.environ.get(flag, False)) | ||
if not DISABLE_LLVM_MISMATCH_ERROR: | ||
raise LLVMVersionMismatchError(msg) |
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.
Could you create a utility function that processes the llvm version mismatches?
I am not sure if raising an exception is an appropriate measure for avoiding possible problems from llvm version mismatches. We have had these mismatches in the past all the time and typically the mismatch of versions is not a problem except for certain use cases. The LLVM 11 vs llvmlite LLVM 14 is the first case where the problem is fatal for most cases (because llvm 14 generates LLVM IR containing features (attributes) that llvm 11 parser does not know how to handle, and even here we can implement a workaround). Here, in the case of 11 vs 14, raising an exception from rbc side is reasonable indeed but for the other cases, I think submitting a warning message with detailed instructions should be sufficient.
The llvm version check should be of the only-once kind and it could be carried out when rbc establishes the connection to the heavydb server. Checking it every time in compile_to_LLVM
would be too much indeed.
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.
@guilhermeleobas since we don't know when heavydb 7 will be release (so merging this PR is blocked), could you move the alloc
releated fixes to a separate PR that can be landed earlier?
31d82dd
to
958678c
Compare
e9f6eab
to
17f2e70
Compare
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 have a couple of suggestions, otherwise looks good!
Thanks, @guilhermeleobas!
rbc/heavydb/remoteheavydb.py
Outdated
s_llvm = '.'.join(map(str, server_llvm_version)) | ||
msg = ( | ||
f'The client LLVM version ({c_llvm}) is greater than the server ' | ||
f'LLVM version ({s_llvm}). This is known to be unsupported. ' |
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.
With #551, I believe this statement has become out-of-date. Could you rephrase the message along the lines of "If you experience any issues that could be due to the discrepancy of LLVM versions, please try to downgrade/update Numba that uses LLVM version ({s_llvm})."
. Also, this statement can be generalized to all LLVM version combinations, not just (11, 14)
.
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 now see that the purpose is to warn only about the problematic LLVM version combinations. Since 11 and 14 are not problematic anymore, this function is essentially a no-op? What do you think?
In fact, the combination of 14 and 11 might be more problematic as a newer client-side LLVM version may use features (such as LLVM attributes or similar) that the server LLVM IR parser cannot handle.
rbc/heavydb/remoteheavydb.py
Outdated
# check LLVM version before compiling to LLVM | ||
flag_name = 'RBC_DISABLE_LLVM_MISMATCH_WARN' | ||
flag = int(os.environ.get(flag_name, False)) | ||
|
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.
We should check the LLVM version and possibly warn only once. One possible approach is as follows:
os.environ[flag_name] = 1 |
…eplace llvm alloca by allocate_varlen_buffer
f95190a
to
89f1b2f
Compare
Array tests are currently brokenFixed