Skip to content
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

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

guilhermeleobas
Copy link
Contributor

@guilhermeleobas guilhermeleobas commented Apr 28, 2023

Array tests are currently broken Fixed

@guilhermeleobas guilhermeleobas added the enhancement New feature or request label Apr 28, 2023
@guilhermeleobas guilhermeleobas self-assigned this Apr 28, 2023
@guilhermeleobas
Copy link
Contributor Author

guilhermeleobas commented May 3, 2023

Pending conda-forge/numba-feedstock#115 Switched to use Numba channel

@guilhermeleobas guilhermeleobas force-pushed the guilhermeleobas/update-numba branch 2 times, most recently from 5078638 to 50fc2f2 Compare May 9, 2023 19:01
@guilhermeleobas
Copy link
Contributor Author

guilhermeleobas commented May 9, 2023

Blocked on HeavyDB 7.0 release on conda-forge as Numba 0.57 uses LLVM 14.

rbc/irtools.py Outdated
Comment on lines 442 to 461
# 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)
Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

environment.yml Show resolved Hide resolved
rbc/irtools.py Outdated
Comment on lines 442 to 461
# 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)
Copy link
Contributor

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.

.github/workflows/rbc_test.yml Show resolved Hide resolved
@guilhermeleobas guilhermeleobas marked this pull request as ready for review May 10, 2023 15:34
Copy link
Contributor

@pearu pearu left a 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?

rbc/heavydb/array.py Show resolved Hide resolved
rbc/heavydb/buffer.py Outdated Show resolved Hide resolved
rbc/heavydb/text_encoding_none.py Outdated Show resolved Hide resolved
Copy link
Contributor

@pearu pearu left a 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/irtools.py Outdated Show resolved Hide resolved
doc/envvars.rst Outdated Show resolved Hide resolved
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. '
Copy link
Contributor

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).

Copy link
Contributor

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.

# check LLVM version before compiling to LLVM
flag_name = 'RBC_DISABLE_LLVM_MISMATCH_WARN'
flag = int(os.environ.get(flag_name, False))

Copy link
Contributor

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:

Suggested change
os.environ[flag_name] = 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants