Conversation
…-sets-into-neo4j-based-handler
…-sets-into-neo4j-based-handler
|
Target CPP Coverage: 64.1421% Target Python Coverage: 97.94% |
…-sets-into-neo4j-based-handler
|
@cwlacewe looks like it got hung up on the UDF tests and fell over. Passed the new PMGDHandler tests though, so yay! Do a restart? Or was there anything obvious that you saw that I should fix? |
@ifadams Yes, in tests/run_tests.sh (lines 68-81) you commented the start of udf servers. If you uncomment those, it should be ok. |
ARGH. I missed that. I'd nuked it when solo testing. Thank youuuu..... |
|
Target CPP Coverage: 64.2822% Target Python Coverage: 97.94% |
Added minio params to test call and restart neo4j
|
Target CPP Coverage: 64.2822% Target Python Coverage: 97.94% |
cwlacewe
left a comment
There was a problem hiding this comment.
Looks good to me. @s-gobriel can you please review?
…-sets-into-neo4j-based-handler
s-gobriel
left a comment
There was a problem hiding this comment.
Looks good to me, only minor comments on the tests
There was a problem hiding this comment.
Should we add more tests to test a pool of connections for descriptors adds/searches. don't see any?
|
|
||
| //Add Descriptor Query | ||
| nr_vecs = 1; | ||
| vec_val = neo4j_generate_desc_linear_increase(dims, nr_vecs,0); |
There was a problem hiding this comment.
Why are we creating separate functions for neo4j vs pmgd to create datasets. I think it is OK, but do not know if it is needed or duplicated code
There was a problem hiding this comment.
lazy fix to avoid refactoring/messing with build. Happy to adjust, or just file a ticket for future refactoring.
There was a problem hiding this comment.
@ifadams would this be a quick adjustment or would it take more time? If it's quick, let's fix it; otherwise, file a ticket
There was a problem hiding this comment.
Lets deal with the initial merge and see where we are. Already running into snags that I'm unclear on the source of, adding new comment below.
|
@rolandoquesada @cwlacewe looks like we're borking on build? I dont think this is from me but I'm less familiar with the container build process. Maybe I missed something in the conflict resolution? #42 ERROR: process "/bin/sh -c git submodule update --init --recursive && sed -i "s|java-11-openjdk|java-17-openjdk|g" /vdms/src/pmgd/java/CMakeLists.txt && sed -i "s|#include <stdio.h>|#include <stdio.h>\n#include |" /vdms/src/pmgd/test/neighbortest.cc && sed -i "s|#include <stdio.h>|#include <stdio.h>\n#include |" /vdms/src/pmgd/tools/mkgraph.cc && mkdir -p /vdms/build && cd /vdms/build && cmake -DCODE_COVERAGE="${BUILD_COVERAGE}" .. && make ${BUILD_THREADS} && echo '#!/bin/bash' > /start.sh && echo 'cd /vdms/build' >> /start.sh && echo 'python /vdms/override_default_config.py -i /vdms/config-vdms.json -o /vdms/build/config-vdms.json' >> /start.sh && echo './vdms' >> /start.sh && chmod 755 /start.sh" did not complete successfully: exit code: 2failed to solve: process "/bin/sh -c git submodule update --init --recursive && sed -i "s|java-11-openjdk|java-17-openjdk|g" /vdms/src/pmgd/java/CMakeLists.txt && sed -i "s|#include <stdio.h>|#include <stdio.h>\n#include |" /vdms/src/pmgd/test/neighbortest.cc && sed -i "s|#include <stdio.h>|#include <stdio.h>\n#include |" /vdms/src/pmgd/tools/mkgraph.cc && mkdir -p /vdms/build && cd /vdms/build && cmake -DCODE_COVERAGE="${BUILD_COVERAGE}" .. && make ${BUILD_THREADS} && echo '#!/bin/bash' > /start.sh && echo 'cd /vdms/build' >> /start.sh && echo 'python /vdms/override_default_config.py -i /vdms/config-vdms.json -o /vdms/build/config-vdms.json' >> /start.sh && echo './vdms' >> /start.sh && chmod 755 /start.sh" did not complete successfully: exit code: 2
|
|
@ifadams error in |
|
@cwlacewe got it, there was a suspicious partial merge, I think I know what it is. Probably shoving this to tomorrow as I have to run momentarily, but will tackle ASAP |
|
Target CPP Coverage: 64.2075% Target Python Coverage: 97.94% |
…-sets-into-neo4j-based-handler
|
Target CPP Coverage: 64.2075% Target Python Coverage: 97.94% |
|
@cwlacewe @s-gobriel we've got everything updated with the new test infrastructure, give it a thumbs up if you're comfortable and trigger the merge? I still need to update docs, will get to that tonight/tomorrow. |
This PR adds the ability to do the following with Neo4J based metadat storage
It currently does not support graph traversal for adding and retrieval of descriptors, but otherwise has feature parity with the existing PMGD based descriptor set support.
Note that I will be continuing cleanup and test development while this PR is open, as this PR is relatively complex and want to be able to parallelize changes as we go along.