Metadata server by rsm0001 · Pull Request #104 · Intel-HLS/GenomicsDB

rsm0001 · 2017-04-20T19:36:32Z

Karthik,
pl. review without merging. Pl. notify Kushal also, if an extra review helps. Any
suggestions to conform to current project coding standards would be appreciated.
Thanks.
Ramesh

kgururaj

Good code.
General comment: Let's replace "meta" with "mapping". "meta" is overloaded so many times in the genomics area that it causes a lot of confusion.

kgururaj · 2017-04-20T20:58:04Z

CMakeLists.txt

 find_library(LIBRT_LIBRARY rt)

+#librt
+find_library(LIBRT_LIBRARY rt)


Duplicate rt?

kgururaj · 2017-04-20T21:00:05Z

CMakeLists.txt

+
+#pgsql driver and dbi libs
+find_package(libdbi REQUIRED)
+include_directories(${LIBDBI_INCLUDE_DIR})


Let's make this an optional dependency - if LIBDBI_FOUND, then a preprocessor macro -DLIBDBI_FOUND should be passed (see add_definitions() for libcsv in the CMakeLists.txt).

In the source code,

#ifdef LIBDBI //All libdbi related code #endif

kgururaj · 2017-04-20T21:00:23Z

CMakeLists.txt

    if(LIBCSV_FOUND)
        target_link_libraries(${target} ${LIBCSV_LIBRARY})
    endif()
+    target_link_libraries(${target} ${LIBDBI_DEV_LIBRARY} ${LIBDBI1_LIBRARY} ${LIBPGSQL_DRIVER_LIBRARY})


Optional dependency

kgururaj · 2017-04-20T21:02:17Z

cmake/Modules/Findlibdbi.cmake

+find_path(LIBDBI_INCLUDE_DIR NAMES dbi.h HINTS "/usr/include/dbi")
+find_library(LIBDBI_DEV_LIBRARY NAMES dbi HINTS "/usr/lib/x86_64-linux-gnu")
+find_library(LIBDBI1_LIBRARY NAMES dbi HINTS "/usr/lib/x86_64-linux-gnu")
+find_library(LIBPGSQL_DRIVER_LIBRARY NAMES dbdpgsql HINTS "/usr/lib/x86_64-linux-gnu/dbd")


These functions must have a variable LIBDBI_ROOT (similar to LIBCSV_DIR) which the user can set to point to the libdbi libraries and headers. This allows users to install the headers/libraries in non-standard locations and still compile GenomicsDB

kgururaj · 2017-04-20T21:02:30Z

cmake/Modules/Findlibdbi.cmake

+find_library(LIBDBI1_LIBRARY NAMES dbi HINTS "/usr/lib/x86_64-linux-gnu")
+find_library(LIBPGSQL_DRIVER_LIBRARY NAMES dbdpgsql HINTS "/usr/lib/x86_64-linux-gnu/dbd")
+include(FindPackageHandleStandardArgs)
+find_package_handle_standard_args(libcsv "Could not find libcsv headers and/or libraries ${DEFAULT_MSG}" LIBDBI_INCLUDE_DIR LIBDBI_DEV_LIBRARY LIBDBI1_LIBRARY LIBPGSQL_DRIVER_LIBRARY)


Change libcsv to libdbi

kgururaj · 2017-04-20T21:05:28Z

src/main/cpp/include/utils/vid_mapper_sql.h

+
+    ~SQLBasedVidMapper();
+  protected:
+    int num_contigs;


No need to store this in the object - use local variable in the function. Once data is loaded into the VidMapper objects m_contig_idx_to_info.size() will give you the number of contigs anyway.

kgururaj · 2017-04-20T21:08:53Z

src/main/cpp/include/utils/vid_mapper_sql.h

+  protected:
+    int num_contigs;
+    dbi_conn conn;
+    dbi_inst instance;


Generally, we distinguish members of a class from local variables with a naming convention.

Google coding style recommends adding "_" at the end of variable name.

In GenomicsDB, I have been using the Hungarian notation - prefix every member variable name with "m_"

Snake case for variable names.

kgururaj · 2017-04-20T21:10:46Z

src/main/cpp/src/utils/vid_mapper_sql.cc

+  dbi_conn_set_option(conn, "username", "postgres");
+  dbi_conn_set_option(conn, "password", "postgres");
+  dbi_conn_set_option(conn, "dbname", "gendb");
+  dbi_conn_set_option(conn, "encoding", "UTF-8");


These should be arguments to the constructor. Other arguments include:

Workspace (string)

Array (string)

kgururaj · 2017-04-20T21:13:12Z

tests/inputs/template_vcf_header.vcf

-##INFO=<ID=MQ0,Number=1,Type=Integer,Description="Total Mapping Quality Zero Reads">
-##INFO=<ID=MQRankSum,Number=1,Type=Float,Description="Z-score From Wilcoxon rank sum test of Alt vs. Ref read mapping qualities">
-##INFO=<ID=ReadPosRankSum,Number=1,Type=Float,Description="Z-score from Wilcoxon rank sum test of Alt vs. Ref read position bias">
-##reference=file:///seq/references/Homo_sapiens_assembly19/v1/Homo_sapiens_assembly19.fasta


Why was this file deleted?

Strange. I find it in my local repository. Let me check.

rsm0001 · 2017-04-21T01:17:16Z

Thank you Karthik! @kgururaj
Will make necessary fixes.

kgururaj

Probably a good time to start adding CI tests in Travis. Let's start by installing the pre-requisites (libdbi-dev and libdbd-pgsql modules) and compile on Travis

kgururaj · 2017-04-27T23:58:42Z

example/CMakeLists.txt

    build_GenomicsDB_executable(example_libtiledb_variant_driver)
    build_GenomicsDB_executable(test_genomicsdb_bcf_generator)
    build_GenomicsDB_executable(test_genomicsdb_importer)
+    build_GenomicsDB_executable(test_mapping_data_loader)


if(LIBDBI_FOUND)
build_GenomicsDB_executable(test_mapping_data_loader)
endif()

kgururaj · 2017-04-27T23:59:41Z

src/main/CMakeLists.txt

    cpp/src/utils/lut.cc
    cpp/src/utils/known_field_info.cc
    cpp/src/utils/vid_mapper.cc
+    cpp/src/utils/vid_mapper_sql.cc


if(LIBDBI_FOUND)

This is not necessary. There is a macro enveloping the code which prevents code from compiling
if libdbi is not installed. Should we remove that check and place this check instead?

Ah ok, that's fine

kgururaj · 2017-04-28T00:02:38Z

src/main/cpp/src/utils/vid_mapper_sql.cc

+  dbi_conn_set_option(m_conn, "encoding", "UTF-8");
+
+  if (dbi_conn_connect(m_conn) < 0) {
+    VERIFY_OR_THROW(1 < 0);


throw exception with a descriptive error message "Could not connect to DB : with "

kgururaj · 2017-04-28T00:06:47Z

src/main/cpp/src/utils/vid_mapper_sql.cc

+    contig_name = dbi_result_get_string(result, "name");
+
+    /**
+     * There is probably a constraint which prevents the following


I don't think so

kgururaj · 2017-04-28T00:37:52Z

src/main/cpp/include/utils/vid_mapper_sql.h

+class SQLBasedVidMapper : public VidMapper {
+  public:
+    SQLBasedVidMapper(const SQLVidMapperRequest&);
+


We should probably delete the copy constructor since there is no way to do deep copy of the libdbi structures.
If you wish to create a move constructor, you can probably copy the libdbi fields to the current object and zero out the other structure.
To delete the copy and move constructors, do:
SQLBasedVidMapper(const SQLBasedVidMapper& other) = delete;
SQLBasedVidMapper(SQLBasedVidMapper&& other) = delete;

That is not a copy constructor. We are passing a request object.

I realize that.

The compiler will always initialize a default copy constructor if you do not explicitly define one or delete the copy constructor. The default copy constructor will likely be incorrect since we have no idea whether the dbi* structures will be copied 'deeply' by an assignment operator. Hence, I'm suggesting to delete the copy and move constructors

kgururaj · 2017-05-01T23:36:25Z

.travis.yml

    - sudo apt-get update -q
    - sudo apt-get install g++-4.9 -y
    - sudo update-alternatives --install /usr/bin/g++ g++ /usr/bin/g++-4.9 60
+    - sudo apt-get install libdbi-dev libdbd-pgsql


apt-get -y install

kdatta · 2017-05-03T02:00:03Z

CMakeLists.txt


+#pgsql driver and dbi libs
+find_package(libdbi)
+if(LIBDBI_FOUND)


shouldn't the libdbi dependence be optional?

It is optional. If libdbi is not installed in an environment, the dependent files won't be compiled.
SQLVidMapper in this instance.

kdatta · 2017-05-03T02:19:39Z

example/src/test_mapping_data_loader.cc

+  MappingDataLoaderTester tester(request);
+
+  tester.print_contig_info();
+


a test code with any assertion is meaningless. Instead of printing these values, why not check whether they match with known values?

kdatta · 2017-05-03T02:23:08Z

src/main/cpp/src/utils/vid_mapper_sql.cc

+
+SQLBasedVidMapper::SQLBasedVidMapper(const SQLVidMapperRequest& request) : VidMapper() {
+  dbi_initialize_r(NULL, &m_instance);
+  m_conn = dbi_conn_new_r("pgsql", m_instance);


don't hardcode the driver name! This goes to the header file as a constant

kdatta · 2017-05-03T02:26:26Z

src/main/cpp/src/utils/vid_mapper_sql.cc

+      continue;
+    }
+
+    int64_t tiledb_column_offset = dbi_result_get_longlong(result, "tiledb_column_offset");


@kgururaj, these field names are hard-coded in many places. better to use macros for all occurrences

kdatta · 2017-05-03T02:27:05Z

tests/load_genomics_metadata.sh

+#vcf_metadata_and_tile
+vcf_only_tile
+echo "--------------------------------------"
+# -----------------------------------------------------------------------------


this looks great

rsm0001 · 2017-05-04T00:44:49Z

@kdatta @kgururaj Pl. review.
Yet to code populating callsets.

kgururaj

Good one - let's add callset mapping query

kgururaj · 2017-05-09T22:08:15Z

src/main/cpp/include/utils/vid_mapper_sql.h

    int load_mapping_data_from_db();

-    std::vector<ContigInfo> get_contigs() { return(m_contig_idx_to_info); }
+    std::vector<ContigInfo>& get_contigs() { return(m_contig_idx_to_info); }


Could we move this to VidMapper?

kgururaj · 2017-05-09T22:08:31Z

src/main/cpp/include/utils/vid_mapper_sql.h

+    std::vector<std::pair<int64_t, int>>& get_contig_end_offsets() {
      return(m_contig_end_2_idx);
    }



Same as above

kgururaj · 2017-05-09T22:08:59Z

src/main/cpp/src/utils/vid_mapper_sql.cc

  ss <<"and (b.name = '" <<m_array_name <<"') and ";
-  ss <<"(a.reference_set_id = b.reference_set_id))";
+  ss <<"(a.reference_set_id = b.reference_set_id)) ";
+  ss <<"order by a.tiledb_column_offset";


👍
let the DB do the work

kdatta · 2017-06-14T23:39:24Z

src/main/cpp/include/utils/vid_mapper_sql.h

+
+    SQLBasedVidMapper&& operator=(SQLBasedVidMapper&&) = delete;
+
+    int load_mapping_data_from_db();


@rsm0001 add documentation to the class functions/members in the header file.

kdatta · 2017-06-14T23:41:25Z

src/main/cpp/src/utils/vid_mapper_sql.cc

+  ss <<"order by a.tiledb_column_offset";
+
+  std::string query = ss.str();
+  std::cout <<"QUERY: <" <<query <<">\n";


delete hanging print statements like this one.

kdatta · 2017-06-15T01:24:02Z

tests/load_genomics_metadata.sh

@@ -0,0 +1,170 @@
+#! /bin/bash


Remove author name. Add Intel MIT license

kdatta · 2017-06-15T01:31:55Z

tests/load_genomics_metadata.sh

+
+# -----------------------------------------------------------------------------
+
+create_load_tile_cfg() {


create_load_tiledb_cfg

kdatta · 2017-06-15T01:32:12Z

tests/load_genomics_metadata.sh

+# -----------------------------------------------------------------------------
+
+create_load_tile_cfg() {
+    outfile="${INITDIR}/load_to_tile.cfg"


rename load_to_tiledb.cfg

kgururaj

Next review phase completed

kgururaj · 2017-06-19T15:57:18Z

cmake/Modules/Findlibdbi.cmake

+
+find_library(LIBPGSQL_DRIVER_LIBRARY NAMES dbdpgsql HINTS "${LIBDBI_DIR}/lib/${CMAKE_C_LIBRARY_ARCHITECTURE}/dbd")
+
+find_library(LIBDBI_DEV_LIBRARY NAMES dbi)


Should the same HINTS be passed for libdbi also?

DEV_LIB is being found without any issues. It was necessary for PGSQL driver because the
library is in a subdirectory (dbd).

I think it's being found because you are installing the libraries in a standard system directory (/usr ?) - if the library is installed in a non-standard location, it wouldn't be detected. Adding HINTS would fix that

Karthik - these are libraries installed thro' package manager. Unless we build libraries from source, we shouldn't have to worry. What do you think?

kgururaj · 2017-06-19T16:26:07Z

src/main/cpp/src/utils/vid_mapper_sql.cc

+
+  while (dbi_result_next_row(result)) {
+    int64_t field_id = dbi_result_get_longlong(result, DBTABLE_FIELD_COLUMN_ID.c_str());
+    int64_t field_idx = (field_id - 1);


field_id returned from the DB could be much larger than the num_fields for this specific array when the mapping DB has information about multiple arrays. Computing field_idx from the field_id will cause writes to un-allocated memory in the lines below. Use an incrementing counter for field_idx.

kgururaj · 2017-06-19T16:27:55Z

src/main/cpp/src/utils/vid_mapper_sql.cc

+    auto known_field_enum = 0u;
+    auto is_known_field = KnownFieldInfo::get_known_field_enum_for_name(field_name, known_field_enum);
+
+    if ((0 == length_type.compare("ERROR")) || length_type.empty()) {


With the std::string, you can use length_type == "ERROR"

True. Let us leave it this way.

kgururaj · 2017-06-19T16:46:39Z

example/src/test_mapping_data_loader.cc

+  get_input("pass_word", "postgres", request.pass_word);
+  get_input("db_name", "gendb", request.db_name);
+
+  get_input("work_space", "/home/rmantrix/git_repos/NomixDB/INST_GenomicsDB/tests/workspace", request.work_space);


Hard-coded path :(
Even if this is default - no hard-coding for workspace, array, please. localhost for mapping DB is fine.

kgururaj · 2017-06-19T16:49:53Z

CMakeLists.txt

+        build_GenomicsDB_executable_common(${target})
+    endif()
+endfunction()
+


Are the tests being called in the CI somewhere? if you use add_test() in CMake, they will be called when "make test" is invoked.

Done. However, CLASSPATH and LD_LIBRARY_PATH are expected in the environment.

Can you modify these environment variables in the .travis.yml file as needed? See the section:

env: global:

You have to install gtest on the Travis VM - add commands to the .travis.yml. The tests didn't build because the library wasn't found.

Also, you should add install(TARGETS ${target} RUNTIME DESTINATION bin) for the gtest executable inside the if block (after line 216)

Will look into it.

Added gtest installation in .travis.yml
LD_LIBRARY_PATH and CLASSPATH already exist
install TARGET is on line 203

kgururaj · 2017-06-19T16:57:35Z

src/main/cpp/src/utils/vid_mapper_sql.cc

+      }
+    }
+
+    std::string combine_op = dbi_result_get_string(result, DBTABLE_FIELD_COLUMN_COMBOP.c_str());


Can the result be null? Based on your PR Intel-HLS/GenomicsSampleAPIs#33, it looks like it can be

True. Fixed.

kgururaj

Final review - if the CI tests pass, can be merged

kgururaj · 2017-06-20T23:33:28Z

CMakeLists.txt

+        build_GenomicsDB_executable_common(${target})
+    endif()
+endfunction()
+


Can you modify these environment variables in the .travis.yml file as needed? See the section:

env: global:

You have to install gtest on the Travis VM - add commands to the .travis.yml. The tests didn't build because the library wasn't found.

kgururaj · 2017-06-20T23:34:48Z

cmake/Modules/Findlibdbi.cmake

+
+find_library(LIBPGSQL_DRIVER_LIBRARY NAMES dbdpgsql HINTS "${LIBDBI_DIR}/lib/${CMAKE_C_LIBRARY_ARCHITECTURE}/dbd")
+
+find_library(LIBDBI_DEV_LIBRARY NAMES dbi)


I think it's being found because you are installing the libraries in a standard system directory (/usr ?) - if the library is installed in a non-standard location, it wouldn't be detected. Adding HINTS would fix that

rsm0001 · 2017-06-21T17:52:02Z

Karthik - these are libraries installed thro' package manager. Unless we build libraries from source, we shouldn't have to worry. What do you think?

kgururaj · 2017-06-22T18:29:40Z

Merge with latest master commit and I will merge the PR into the master

…csDB into metadata_server

Ramesh Mantri added 7 commits April 16, 2017 11:53

cmake libdbi changes and script to initialize metadata

301643e

restructuring tests directory

9b00cd8

sql vid mapper - first cut

0f4ed04

temporary changes for tests

a83692b

Merge branch 'master' into metadata_server

1764668

testing retrieval

3f11a23

printing out contigs in metadata loader tester

3d491cd

rsm0001 requested a review from kgururaj April 20, 2017 19:36

kgururaj reviewed Apr 20, 2017

View reviewed changes

rsm0001 assigned kgururaj Apr 21, 2017

Ramesh Mantri added 3 commits April 21, 2017 12:54

fixes suggested in review

974b7b7

libdbi should be optional

a5e4d62

file was somehow deleted

8c89eff

kgururaj reviewed Apr 28, 2017

View reviewed changes

installing dbi libs thro travis

27d0764

kgururaj reviewed May 1, 2017

View reviewed changes

reference query modified

cf55c30

kdatta reviewed May 3, 2017

View reviewed changes

incorporating changes suggested in code review

faec345

rsm0001 assigned kdatta May 4, 2017

tentative code to load callsets info

e79dd50

kgururaj reviewed May 9, 2017

View reviewed changes

Ramesh Mantri added 2 commits May 11, 2017 11:27

modified callset query

c0b5860

using google test

107ce14

Ramesh Mantri added 2 commits June 7, 2017 11:34

merged from master

8f2b3b0

creating include directory for GTests

b104f90

kdatta suggested changes Jun 15, 2017

View reviewed changes

changes recommended in code review

3f4af3d

kgururaj suggested changes Jun 19, 2017

View reviewed changes

kgururaj reviewed Jun 19, 2017

View reviewed changes

Ramesh Mantri added 2 commits June 20, 2017 09:42

changes according to code review

c6e1ee6

Merge branch 'master' into metadata_server

38f6fe3

kgururaj approved these changes Jun 20, 2017

View reviewed changes

Ramesh Mantri added 5 commits June 21, 2017 11:22

adding gtest to travis

14af5d4

temp commit to verify gtest install

a712fda

building gtest for 14.04 since package doesnt exist for 14.04

9a89695

gtests will be run inside docker

6dab698

fixing typo

f6eace7

Ramesh Mantri added 11 commits June 22, 2017 15:17

testing postgresql service

ebde3f3

testing psql in travis

8a04962

restoring GTests

2f2c500

restoring GTests

ce2411c

restoring GTests

f1ba868

restoring GTests

7a38909

trying to get gtests to run in travis

1ddfb53

Merge branch 'master' into metadata_server

073e46f

gtests

cbaa64c

Merge branch 'metadata_server' of https://github.com/Intel-HLS/Genomi…

4de9eda

…csDB into metadata_server

Tests finalized for now

24ca93c

kdatta approved these changes Jun 27, 2017

View reviewed changes

kgururaj merged commit 8a2ff80 into master Jun 27, 2017

kgururaj deleted the metadata_server branch June 27, 2017 18:20

		MappingDataLoaderTester tester(request);

		tester.print_contig_info();


		SQLBasedVidMapper&& operator=(SQLBasedVidMapper&&) = delete;

		int load_mapping_data_from_db();


		# -----------------------------------------------------------------------------

		create_load_tile_cfg() {


		find_library(LIBPGSQL_DRIVER_LIBRARY NAMES dbdpgsql HINTS "${LIBDBI_DIR}/lib/${CMAKE_C_LIBRARY_ARCHITECTURE}/dbd")

		find_library(LIBDBI_DEV_LIBRARY NAMES dbi)

Conversation

rsm0001 commented Apr 20, 2017

Uh oh!

kgururaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsm0001 commented Apr 21, 2017

Uh oh!

kgururaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsm0001 commented May 4, 2017

Uh oh!

kgururaj left a comment