Bugfix 1900 #597
Bugfix 1900 #597
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughRemoved the Ravel class's internal mutex and all locking constructs; updated two submodule commit pointers; minor whitespace adjustments in a test file. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Pull request overview
This PR addresses bugfix 1900 by fixing test assertions to properly handle sparse tensor indices and updating the version to 3.21.0-beta.4.
Changes:
- Fixed test assertions in
testTensorOps.ccto use sorted indices when validating sparse tensor data - Updated version strings across multiple files from 3.20.0 to 3.21.0-beta.4
- Enabled OpenMP by default (except on Darwin/macOS) and improved build configuration
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/testTensorOps.cc | Fixed test assertions to properly handle unsorted sparse tensor indices by sorting before comparison |
| test/Makefile | Commented out OPENMP conditional compilation flags |
| resetRavel.py | Changed output to print basename instead of full path |
| model/ravelWrap.h | Added shared_mutex include |
| model/ravelWrap.cc | Reformatted function bodies and fixed indentation |
| minskyVersion.h | Updated version to 3.21.0-beta.4 |
| gui-js/package.json | Updated version to 3.21.0-beta.4 |
| gui-js/libs/shared/src/lib/constants/version.ts | Updated version to 3.21.0-beta.4 |
| gui-js/libs/shared/src/lib/backend/minsky.ts | Removed unused Image method |
| engine/minskyTensorOps.cc | Refactored conditional logic to reduce nesting |
| ecolab | Updated subproject commit |
| doc/version.tex | Updated version to 3.21.0-beta.4 |
| RavelCAPI | Updated subproject commit |
| Makefile | Enabled OpenMP by default except on Darwin, added CPUPROFILE support, and added libgomp-1 to Windows dependencies |
| Doxyversion | Updated version to 3.21.0-beta.4 |
Files not reviewed (1)
- gui-js/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/testTensorOps.cc
Outdated
| for (size_t _i=0; _i<9; ++_i) EXPECT_EQ(expected[_i], (*chain.back())[_i]); | ||
| index=chain.back()->index(); | ||
| sortedIdx=vector<size_t>(index.begin(),index.end()); sort(sortedIdx.begin(),sortedIdx.end()); | ||
| for (size_t _i=0; _i<9; ++_i) EXPECT_EQ(expected[_i], chain.back()->atHCIndex(sortedIdx[_i])); |
There was a problem hiding this comment.
Inconsistent indentation: this line has an extra leading space compared to the identical patterns on lines 1201, 1215, and 1222. All similar for-loop statements should have consistent indentation.
| for (size_t _i=0; _i<9; ++_i) EXPECT_EQ(expected[_i], chain.back()->atHCIndex(sortedIdx[_i])); | |
| for (size_t _i=0; _i<9; ++_i) EXPECT_EQ(expected[_i], chain.back()->atHCIndex(sortedIdx[_i])); |
test/testTensorOps.cc
Outdated
| for (size_t _i=0; _i<5; ++_i) EXPECT_EQ(expectedf[_i], (*chain.back())[_i]); | ||
| index=chain.back()->index(); | ||
| sortedIdx=vector<size_t>(index.begin(),index.end()); sort(sortedIdx.begin(),sortedIdx.end()); | ||
| for (size_t _i=0; _i<5; ++_i) EXPECT_EQ(expectedf[_i], chain.back()->atHCIndex(sortedIdx[_i])); |
There was a problem hiding this comment.
Trailing whitespace detected at the end of this line. Remove the trailing space for consistency with the codebase.
| for (size_t _i=0; _i<5; ++_i) EXPECT_EQ(expectedf[_i], chain.back()->atHCIndex(sortedIdx[_i])); | |
| for (size_t _i=0; _i<5; ++_i) EXPECT_EQ(expectedf[_i], chain.back()->atHCIndex(sortedIdx[_i])); |
| endif | ||
|
|
||
| MAKEOVERRIDES+=FPIC=1 CLASSDESC=$(shell pwd)/ecolab/classdesc/classdesc EXTRA_FLAGS="$(EXTRA_FLAGS)" CPLUSPLUS="$(CPLUSPLUS)" GCOV=$(GCOV) OPENMP=1 | ||
| $(warning $(MAKEOVERRIDES)) |
There was a problem hiding this comment.
Debug warning statement left in production code. This warning output should be removed or commented out before merging to avoid cluttering build logs.
| $(warning $(MAKEOVERRIDES)) | |
| # $(warning $(MAKEOVERRIDES)) |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@RavelCAPI`:
- Line 1: Remove the unused include by deleting the line '#include
<shared_mutex>' from ravelWrap.h (and ensure ravelWrap.cc does not require it),
and run a build to confirm no compile errors; then verify the RavelCAPI
submodule pointer update is intentional—if there are no threading/mutex changes
referenced in ravelWrap.h or ravelWrap.cc, revert the submodule update or add a
commit message documenting why the pointer bump is needed.
| @@ -1 +1 @@ | |||
| Subproject commit 8282d1b91ec193f3b0e4d6102eaa9873814d3994 | |||
| Subproject commit b6974785f1e168028805e15527d3843f07b84fbf | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if RavelCAPI submodule exists and is initialized
if [ -d "RavelCAPI" ]; then
echo "=== RavelCAPI submodule directory exists ==="
ls -la RavelCAPI/.git 2>/dev/null && echo "Submodule is initialized" || echo "Submodule not initialized"
# Try to retrieve information about the submodule
echo ""
echo "=== Current submodule status ==="
git config --file .gitmodules --get-regexp path | grep -i ravel
# Check the submodule HEAD
echo ""
echo "=== Current commit in RavelCAPI ==="
cd RavelCAPI && git rev-parse HEAD 2>/dev/null || echo "Cannot determine current commit"
cd - > /dev/null
else
echo "RavelCAPI directory does not exist"
fi
# Check main repository changes related to mutex
echo ""
echo "=== Files changed in main PR (looking for ravelWrap) ==="
git diff --name-only HEAD~1 HEAD 2>/dev/null | grep -i ravel || echo "Cannot determine changed files"
# Try to see if there's information about this change in git index
echo ""
echo "=== Checking git index for submodule change ==="
git ls-files --stage | grep RavelCAPIRepository: highperformancecoder/minsky
Length of output: 486
🏁 Script executed:
#!/bin/bash
# Check the current directory structure
echo "=== Repository structure ==="
git ls-files | head -20
# Look for ravelWrap files mentioned in the review context
echo ""
echo "=== Searching for ravelWrap files ==="
find . -name "*ravelWrap*" -type f 2>/dev/null
# Check if there are any mutex-related changes in the current commit
echo ""
echo "=== Looking for mutex-related changes ==="
git diff HEAD~1 HEAD 2>/dev/null | grep -i -E "(mutex|lock|sync)" | head -20 || echo "Cannot retrieve diffs or no mutex changes found"Repository: highperformancecoder/minsky
Length of output: 639
🏁 Script executed:
#!/bin/bash
# Check the size of ravelWrap files first
echo "=== File sizes ==="
wc -l ./model/ravelWrap.cc ./model/ravelWrap.h
echo ""
echo "=== Searching for mutex in ravelWrap.cc ==="
grep -n "mutex" ./model/ravelWrap.cc | head -20
echo ""
echo "=== Searching for mutex in ravelWrap.h ==="
grep -n "mutex" ./model/ravelWrap.h | head -20
echo ""
echo "=== Checking git status ==="
git status
echo ""
echo "=== Checking git log for recent commits ==="
git log --oneline -10Repository: highperformancecoder/minsky
Length of output: 453
🏁 Script executed:
#!/bin/bash
# Check if we can see staged changes
echo "=== Staged changes ==="
git diff --cached --name-only
echo ""
echo "=== All changes (staged and unstaged) ==="
git diff HEAD --name-only 2>/dev/null | head -20
echo ""
echo "=== Check if there's a diff in the working tree ==="
git status -s
echo ""
echo "=== Try to get commit message ==="
git log -1 --pretty=format:"%B"Repository: highperformancecoder/minsky
Length of output: 258
🏁 Script executed:
#!/bin/bash
# Let's examine the actual content of ravelWrap files
echo "=== Content of ravelWrap.h ==="
cat -n ./model/ravelWrap.h
echo ""
echo "=== Content of ravelWrap.cc ==="
cat -n ./model/ravelWrap.ccRepository: highperformancecoder/minsky
Length of output: 48965
🏁 Script executed:
#!/bin/bash
# Check .gitmodules to understand the submodule setup
echo "=== .gitmodules content ==="
cat .gitmodules
echo ""
echo "=== Check if there's a remote URL for the submodule ==="
git config --file .gitmodules --get submodule.RavelCAPI.url
echo ""
echo "=== Check git log to understand recent commits ==="
git log --oneline -20 | head -20
echo ""
echo "=== Look for any reference to RavelCAPI in recent commits ==="
git log --oneline --all | grep -i ravel | head -10Repository: highperformancecoder/minsky
Length of output: 667
Remove the unused #include <shared_mutex> from ravelWrap.h and verify the RavelCAPI submodule update is intentional.
The header includes <shared_mutex> but it is never used in either ravelWrap.h or ravelWrap.cc. Additionally, clarify the purpose of updating the RavelCAPI submodule pointer since there are no active mutex synchronization changes in the main codebase to justify this update as part of a threading-related fix.
🤖 Prompt for AI Agents
In `@RavelCAPI` at line 1, Remove the unused include by deleting the line
'#include <shared_mutex>' from ravelWrap.h (and ensure ravelWrap.cc does not
require it), and run a build to confirm no compile errors; then verify the
RavelCAPI submodule pointer update is intentional—if there are no
threading/mutex changes referenced in ravelWrap.h or ravelWrap.cc, revert the
submodule update or add a commit message documenting why the pointer bump is
needed.
There was a problem hiding this comment.
@coderabbitai The updated RavelCAPI reference contains mutex syncronisation changes??
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the RavelCAPI submodule changes between the old and new commits
echo "=== Checking RavelCAPI submodule change details ==="
# Initialize and update submodule if needed
git submodule update --init RavelCAPI 2>/dev/null || echo "Submodule already initialized"
cd RavelCAPI
echo ""
echo "=== Current commit in RavelCAPI ==="
git rev-parse HEAD
echo ""
echo "=== Log between old and new commit ==="
git log --oneline 8282d1b91ec193f3b0e4d6102eaa9873814d3994..b6974785f1e168028805e15527d3843f07b84fbf 2>/dev/null || \
git log --oneline 8282d1b..b6974785 2>/dev/null || \
echo "Could not retrieve log"
echo ""
echo "=== Checking for mutex-related changes in RavelCAPI ==="
git diff 8282d1b91ec193f3b0e4d6102eaa9873814d3994..b6974785f1e168028805e15527d3843f07b84fbf --stat 2>/dev/null | head -20 || \
git diff 8282d1b..b6974785 --stat 2>/dev/null | head -20 || \
echo "Could not retrieve diff stats"
echo ""
echo "=== Searching for mutex in the diff ==="
git diff 8282d1b91ec193f3b0e4d6102eaa9873814d3994..b6974785f1e168028805e15527d3843f07b84fbf 2>/dev/null | grep -i -C 3 "mutex" | head -50 || \
git diff 8282d1b..b6974785 2>/dev/null | grep -i -C 3 "mutex" | head -50 || \
echo "Could not search for mutex changes"
echo ""
echo "=== List all files in RavelCAPI ==="
ls -la
Length of output: 3772
@highperformancecoder, you're absolutely right, and I apologize for the confusion. The RavelCAPI submodule update does indeed contain mutex synchronization changes:
Commit b697478: "Add a shared_mutex around calls to RavelCAPI to make ravelCAPI::Ravel and ravelCAPI::Database thread-safe. For Minsky #1900"
The submodule adds an updateMutex (shared_lock and lock_guard) to numerous methods in dynamicRavelCAPI.cc, including:
render(),clear(),rescale(),radius(),rank(),description()- Mouse event handlers:
onMouseDown(),onMouseUp(),onMouseMotion(),onMouseOver(),onMouseLeave()
This refactoring moves the thread safety from the Minsky wrapper layer (where updateMutex was removed from ravelWrap.h/.cc) into the RavelCAPI library itself. This is a cleaner design since the synchronization is now encapsulated within RavelCAPI rather than being the responsibility of the wrapper.
My original review comment was incorrect. The submodule update is intentional and directly addresses the threading issues in bug #1900.
This change is
Summary by CodeRabbit
Chores
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.