fix blis dynamic lib name, avoid bli_init output at runtime#689
fix blis dynamic lib name, avoid bli_init output at runtime#689bung87 wants to merge 1 commit intomratsim:masterfrom
blis dynamic lib name, avoid bli_init output at runtime#689Conversation
Summary of ChangesHello @bung87, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the integration of the BLIS backend by addressing two key areas: suppressing verbose initialization output and improving the robustness of dynamic library loading. These changes enhance the user experience by reducing console clutter and bolster cross-platform compatibility for the BLIS library. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughThis PR refines BLIS backend integration by removing initialization status output while preserving the initialization call, and adding cross-platform dynamic library naming support (Windows, macOS, Linux) with a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the dynamic library name for BLIS to correctly support different operating systems (Windows, macOS, Linux) and suppresses the output from bli_init during initialization. While the library name fix is good, suppressing the bli_init output by discarding its return value is risky as it hides potential initialization errors. I've added a suggestion to properly handle the return value. I've also suggested a small refactoring to make the platform-specific constants more concise.
| static: echo "--USING BLIS--" | ||
| include ./blis_api | ||
| echo "Blis initialization status: " & $bli_init() | ||
| discard bli_init() |
There was a problem hiding this comment.
By discarding the return value of bli_init(), you are ignoring potential initialization errors. This could lead to silent failures at runtime if BLIS fails to initialize correctly. It's better to check the return code and raise an exception on failure to ensure the library is properly initialized before use.
let err = bli_init()
doAssert err == BLIS_SUCCESS, "BLIS initialization failed with error: " & $err
| when defined(windows): | ||
| const blisPrefix = "" | ||
| else: | ||
| const blisPrefix = "lib" | ||
|
|
||
| when defined(windows): | ||
| const blisSuffix = ".dll" | ||
| elif defined(macosx): | ||
| const blisSuffix = ".dylib" | ||
| else: | ||
| const blisSuffix = ".so" #MacOS & Linux | ||
| const blisSuffix = ".so" |
There was a problem hiding this comment.
These two when blocks can be combined into a single, more concise expression using a tuple assignment. This improves readability and reduces code duplication.
const (blisPrefix, blisSuffix) = when defined(windows):
("", ".dll")
elif defined(macosx):
("lib", ".dylib")
else: # Linux and others
("lib", ".so")
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/arraymancer/tensor/backend/blis_api.nim (1)
140-140:⚠️ Potential issue | 🟡 MinorRemove the unused
bli_is_initializedbinding or document future use.The binding is currently unused in the codebase. While the Nim
boolreturn type correctly maps to BLIS'sbool_t(which is C99boolin modern BLIS, post-July 2020), the lack of a minimum BLIS version constraint in the project means older versions withbool_tas an integer type could theoretically be used. If this binding is not needed, remove it; if planned for future use, add a comment explaining the intent.
🧹 Nitpick comments (1)
src/arraymancer/tensor/backend/blis.nim (1)
18-18: Consider checkingbli_init()result at least in debug mode.The init result is now silently discarded. If initialization fails, downstream BLAS calls will fail with no diagnostic. For consistency with the
bli_finalize()handling on lines 21-24, consider:Suggested change
- discard bli_init() + when defined(debug): + echo "Blis initialization status: " & $bli_init() + else: + discard bli_init()
Greptile SummaryThis PR fixes two real cross-platform bugs in the BLIS backend: the library filename on Windows (
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant App
participant blis.nim
participant blis_api.nim
participant libblis (dynamic)
App->>blis.nim: module loaded (when defined(blis))
blis.nim->>blis_api.nim: include (resolves libblis path per OS)
Note over blis_api.nim: Windows: blis.dll<br/>macOS: libblis.dylib<br/>Linux: libblis.so
blis.nim->>libblis (dynamic): bli_init() [return discarded]
blis.nim->>App: ready
App-->>libblis (dynamic): bli_gemm / bli_gemv calls
App->>blis.nim: process exit → quit_blis()
blis.nim->>libblis (dynamic): bli_finalize() [logged in debug mode]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| static: echo "--USING BLIS--" | ||
| include ./blis_api | ||
| echo "Blis initialization status: " & $bli_init() | ||
| discard bli_init() |
There was a problem hiding this comment.
Initialization error silently discarded in all build modes
bli_init() returns a BlisError that can signal BLIS_FAILURE (-2). The existing quit_blis proc correctly logs the result under defined(debug) and only discards in release mode. The new discard bli_init() deviates from this pattern and drops the return value unconditionally — so a failed initialization will produce no warning even in debug builds, leading to confusing downstream errors.
Consider mirroring the same debug/release pattern already used for bli_finalize():
| discard bli_init() | |
| when defined(debug): | |
| echo "Blis initialization status: " & $bli_init() | |
| else: | |
| discard bli_init() |
1 participant
Summary by CodeRabbit
Bug Fixes
Improvements