Community detection memory tracking #431
Conversation
imilinovic
changed the title
imilinovic
added
Docs unnecessary
|
@imilinovic, quick question, what's the issue related? Can you link it? 👀 |
… into community-detection-limits
…into community-detection-limits
imilinovic
added
status: ready
| # Module tests | ||
| if (NOT MAGE_CUGRAPH_ENABLE) | ||
| include(GoogleTest) | ||
| set(community_detection_test_src | ||
| community_detection_test.cpp | ||
| algorithm/louvain.cpp) | ||
|
|
||
| add_executable(community_detection_test "${community_detection_test_src}") | ||
| target_include_directories(community_detection_test PRIVATE ${GRAPPOLO_HEADERS} ${CMAKE_CURRENT_SOURCE_DIR}) | ||
| target_link_libraries(community_detection_test PRIVATE mg_utility basic_cd full_syn_opt coloring util OpenMP::OpenMP_CXX mage_gtest) | ||
| gtest_add_tests(TARGET community_detection_test) | ||
| endif() | ||
|
|
||
|
|
There was a problem hiding this comment.
These tests were ported to e2e tests.
The tests were not usable because now community_detection library uses mgp_graph and the graph testing library doesn't use that.
There was a problem hiding this comment.
Looks good to me. Only a small note.
As I understand the tests you moved to e2e are tests that existed before and passed, even though community detection algorithms weren't usable.
It would be good to add test that will check if the memory limit is being respected with introduced changes.
There was a problem hiding this comment.
Good job (& thanks for the reviewers’ note)!
I agree with @DavIvek that it would be good to have a test to verify that this query module now respects the memory limits. Also double-check if there remain any (de)allocations that aren’t tracked.
NB: Is it possible to disable SonarCloud warnings for this PR alone? While it’s a useful tool, its recommendations aren’t exactly relevant to this PR:
- repeated code: this PR mainly consists of adding memory tracking calls
- cognitive complexity of functions: not applicable here because all code save for the memory tracking calls is third-party
|
|
I think the test to check that this works can't be done because of the following: Memory limit is always being respected but the problem was that memory counting was wrong. The only way would be to compare it with the system memory (or see if the system crashes when setting the limit close to available RAM on the machine) which I don't think is possible at least not with the current MAGE testing infrastructure. I think sonarcloud can just be ignored here and merge if all other tests pass. |
Description
Community detection memory tracking was not working properly when multiple threads were used so it was fixed.
Add thread memory tracking to community detection.
Requires PR436 and PR1631 before merging
Note to reviewer (!!!):
Review with suffix
?w=1to ignore whitespaces changes which are automatically made by IDE (and there are a lot of them).Link for reviewing
Pull request type
Release note:
Community detection now respects memory limits and doesn't crash database.
Related issues
closes #379
closes #219
closes memgraph/memgraph#955
######################################
Reviewer checklist (the reviewer checks this part)
Module/Algorithm
######################################