Skip to content

update bnb log #1661

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

update bnb log #1661

wants to merge 8 commits into from

Conversation

jiqing-feng
Copy link
Contributor

@jiqing-feng jiqing-feng commented May 29, 2025 •
edited
Loading

Update the error message when xpu is available, we don't need native library on XPU
@matthewdouglas Please review it. Thanks!

yao-matrix
yao-matrix reviewed May 29, 2025
View reviewed changes
bitsandbytes/cextension.py Outdated
"IPEX is recommended for Intel XPU support in bitsandbytes to get better performance. "
"Please check the installation doc to install `intel_extension_for_pytorch`. "
)
lib = ErrorHandlerMockBNBNativeLibrary("XPU does not need native library")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why need ErrorHandlerMockBNBNativeLibrary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it was the default lib, but we can just set lib=None since XPU will not use lib.

@jiqing-feng jiqing-feng marked this pull request as ready for review May 29, 2025 05:33
Devjiu
Devjiu reviewed May 29, 2025
View reviewed changes
bitsandbytes/cextension.py Outdated
if not ipex_xpu:
logger.warning(
"Detected Intel XPU but no Intel Extension for PyTorch (IPEX) installed. "
"IPEX is recommended for Intel XPU support in bitsandbytes to get better performance. "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please mention Triton here?
For example: "XPU is detected, but IPEX is missing, will try to use Trtiton implementation. If performance is lower your expectations, consider installing IPEX"

The current wording effectively prohibits the use of XPU without IPEX.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@jiqing-feng
update bnb log
343bfd5 
Signed-off-by: jiqing-feng <[email protected]>
@jiqing-feng
fix lib
ccc645d 
Signed-off-by: jiqing-feng <[email protected]>
@jiqing-feng
update warning
09ceea2 
Signed-off-by: jiqing-feng <[email protected]>
@matthewdouglas matthewdouglas added this to the v0.47.0 milestone Jun 2, 2025
@matthewdouglas matthewdouglas self-requested a review June 2, 2025 19:29
@matthewdouglas matthewdouglas added the Low Risk Risk of bugs in transformers and other libraries label Jun 2, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 2, 2025

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@jiqing-feng jiqing-feng marked this pull request as draft June 3, 2025 05:13
@jiqing-feng jiqing-feng marked this pull request as ready for review June 3, 2025 05:49
@jiqing-feng
Copy link
Contributor Author

jiqing-feng commented Jun 3, 2025 •
edited
Loading

We need to use mock lib because of the functional variable.

@jiqing-feng
Copy link
Contributor Author

Hi @matthewdouglas . This PR is ready to be reviewed, please let me know your comments. Thanks!

@Egor-Krivov
Copy link
Contributor

I don't think we should push user into a specific XPU backend, until we clearly know which one is likely faster.
I just run simple inference benchmark and couldn't get performance improvements from IPEX: #1629 (comment)

@jiqing-feng
Copy link
Contributor Author

I don't think we should push user into a specific XPU backend, until we clearly know which one is likely faster. I just run simple inference benchmark and couldn't get performance improvements from IPEX: #1629 (comment)

You were right, I have changed the log. This PR will avoid xpu through load native library error, as we don't want to load native lib on XPU. Please review my new changes. Without this PR, the error be like:

Traceback (most recent call last):
  File "/usr/local/lib/python3.11/dist-packages/bitsandbytes/cextension.py", line 115, in <module>
    lib = get_native_library()
          ^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/dist-packages/bitsandbytes/cextension.py", line 86, in get_native_library
    dll = ct.cdll.LoadLibrary(str(binary_path))
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/ctypes/__init__.py", line 454, in LoadLibrary
    return self._dlltype(name)
           ^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/ctypes/__init__.py", line 376, in __init__
    self._handle = _dlopen(self._name, mode)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^
OSError: /usr/local/lib/python3.11/dist-packages/bitsandbytes/libbitsandbytes_cpu.so: cannot open shared object file: No such file or directory

@jiqing-feng jiqing-feng marked this pull request as draft June 5, 2025 07:04
@jiqing-feng
update log
bc4a540 
Signed-off-by: jiqing-feng <[email protected]>
matthewdouglas
matthewdouglas reviewed Jun 5, 2025
View reviewed changes
bitsandbytes/cextension.py
"Detected Intel XPU without intel_extension_for_pytorch installed. Triton implementation will be used."
)
# create a mock with error messaging as fallback
lib = ErrorHandlerMockBNBNativeLibrary("XPU does not need library loading")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't we still want it in case the user has a reason to use the two CPU ops it implements? We don't necessarily know they won't just because an XPU is available.

matthewdouglas
matthewdouglas reviewed Jun 5, 2025
View reviewed changes
bitsandbytes/cextension.py
Comment on lines +303 to +305
logger.warning(
"Detected Intel XPU without intel_extension_for_pytorch installed. Triton implementation will be used."
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For most of our target audience I feel like this isn't going to be super informative. It's OK for now but eventually we might want to find a way to spell out exactly what it means for them, and whether we should be loud about it or not.

@jiqing-feng
Copy link
Contributor Author

Close this PR as we may change the XPU behavior in the future.

@jiqing-feng jiqing-feng closed this Jun 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yao-matrix yao-matrix yao-matrix left review comments

@Devjiu Devjiu Devjiu left review comments

@matthewdouglas matthewdouglas matthewdouglas left review comments

Assignees
No one assigned
Labels
Intel Low Risk Risk of bugs in transformers and other libraries
Projects
None yet
Milestone
v0.47.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jiqing-feng @Egor-Krivov @yao-matrix @Devjiu @matthewdouglas