-
Notifications
You must be signed in to change notification settings - Fork 637
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
fix library detection #873
fix library detection #873
Conversation
b27420e
to
0961eba
Compare
1a8d409
to
c1d3209
Compare
rebased. fix some mistakes. partial squashed. |
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.
Thank you for this PR! Overall it looks very good. There are some issues that I would like more information on but I think we would be able to merge this without any major issues.
@@ -46,26 +32,31 @@ def generate_bug_report_information(): | |||
print('') | |||
|
|||
if 'CONDA_PREFIX' in os.environ: | |||
paths = find_file_recursive(os.environ['CONDA_PREFIX'], '*cuda*so') | |||
paths = find_file_recursive(os.environ['CONDA_PREFIX'], '*cuda*') |
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 need to make sure that this does not match non-binary files. How can we make sure that this is correct in most environments?
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.
dll
, so
, or dylib
ext part appended later in the find_file_recursive()
|
||
return out | ||
return outs |
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.
Does the list instead of path cause errors in some edge cases? As I understand it, we either have dll
or dylib
, is that correct? Or can we have both in some cases?
@@ -111,7 +118,9 @@ def manual_override(self): | |||
'For example by adding the following to your .bashrc: export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:<path_to_cuda_dir/lib64\n' | |||
f'Loading CUDA version: BNB_CUDA_VERSION={os.environ["BNB_CUDA_VERSION"]}' | |||
f'\n{"="*80}\n\n')) | |||
self.binary_name = self.binary_name[:-6] + f'{os.environ["BNB_CUDA_VERSION"]}.so' | |||
binary_name = self.binary_name.rsplit(".", 1)[0] |
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.
This is a bit of a mess (based on my messy previous code). I have not looked at the details. Is there a way where we can clean this up slightly?
10c8f41
to
6dae722
Compare
rebased with some fixes. |
Signed-off-by: Won-Kyu Park <[email protected]>
* use os.pathsep
rebased, partial squashed, conflict resolved |
@wkpark Thanks a lot for the nice PR and your continuing great work around bnb, despite frustrations! The code looks really good. From what I can see the changes are mostly in the reporting part and therefore not risky. The other changes are very minimal and seem unproblematic by visual inspection. I think we can take the risk to merge this. I wish we had a better way of testing the setup module though. It would profit from further refactors and that would make it easier. Thanks for your valuable contribution! |
e651e8e
into
bitsandbytes-foundation:main
out = glob.glob(os.path.join(folder, "**", filename + ext)) | ||
outs.extend(out) | ||
except Exception as e: | ||
raise RuntimeError('Error: Something when wrong when trying to find file. {e}') |
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.
This is missing an f
so the exception is never printed. (That's fixed in #984.)
fix library detection
manually cherry-picked from PR #788
See also #876