Finished DataMan Transport Manager refactoring but full functionality still pending on Read API #270

JasonRuonanWang · 2017-09-19T18:33:49Z

No description provided.

…y still pending on Read API

JasonRuonanWang · 2017-09-19T19:21:31Z

@chuckatkins I got this continuous-integration failed but haven't got any idea what it is about. Could you have a look at your convenience? Thanks.

chuckatkins · 2017-09-19T19:29:17Z

Check the cdash link in the status for the build results. you should be able to see the build failures on Windows from the.

williamfgc

Great job, overall! Just minor stuff to be compliant with the coding standard. Thanks!

williamfgc · 2017-09-20T17:24:12Z

source/adios2/engine/dataman/DataManWriter.tcc

@@ -40,15 +40,21 @@ void DataManWriter::DoWriteCommon(Variable<T> &variable, const T *values)
        variable.m_Start.assign(variable.m_Count.size(), 0);
    }

-    json jmsg;
+    std::cout << "DoWriteCommon begin" << std::endl;


Please, put all std::cout inside some sort of if(rank == 0)

williamfgc · 2017-09-20T17:24:35Z

source/adios2/engine/dataman/DataManWriter.tcc

+        std::accumulate(variable.m_Shape.begin(), variable.m_Shape.end(),
+                        sizeof(T), std::multiplies<size_t>());
+
+    std::cout << "DoWriteCommon end" << std::endl;


Same here, inside rank zero.

williamfgc · 2017-09-20T17:26:10Z

source/adios2/toolkit/transport/wan/WANZmq.cpp

@@ -51,7 +56,8 @@ void WANZmq::Open(const std::string &name, const OpenMode openMode)

        m_Socket = zmq_socket(m_Context, ZMQ_REQ);
        const std::string fullIP("tcp://" + m_IPAddress + ":" + m_Port);
-        zmq_connect(m_Socket, fullIP.c_str());
+        std::cout << "full IP = " << fullIP << std::endl;


cout in rank zero

williamfgc · 2017-09-20T17:26:21Z

source/adios2/toolkit/transport/wan/WANZmq.cpp

@@ -78,6 +84,7 @@ void WANZmq::Open(const std::string &name, const OpenMode openMode)

        m_Socket = zmq_socket(m_Context, ZMQ_REP);
        const std::string fullIP("tcp://" + m_IPAddress + ":" + m_Port);
+        std::cout << "full IP = " << fullIP << std::endl;


cout in rank 0

williamfgc · 2017-09-20T17:29:01Z

source/adios2/toolkit/transportman/dataman/DataMan.cpp


        const std::string trans(
            GetParameter("transport", parameters, true, m_DebugMode, ""));

        const std::string ipAddress(
            GetParameter("ipaddress", parameters, true, m_DebugMode, ""));

-        std::string port(
+        std::string port_control(


Let's avoid _ in names. Let's use only one style: camelCase....e.g portControl. C libraries use that, so if we see this we know it's coming from an external C library (e.g. MPI)

williamfgc · 2017-09-20T18:13:09Z

source/adios2/engine/dataman/DataManReader.cpp

+            para[i]["name"] = "stream";
+            para[i]["ipaddress"] = "127.0.0.1";
+        }
+        m_Man.OpenWANTransports("zmq", adios2::OpenMode::Read, para, true);

        std::string method_type;


switch to camelCase: methodType https://github.com/ornladios/ADIOS2/wiki/Variables-Scope,-Functions,-and-Namespaces

williamfgc · 2017-09-20T18:13:42Z

source/adios2/engine/dataman/DataManReader.cpp

+            para[i]["name"] = "stream";
+            para[i]["ipaddress"] = "127.0.0.1";
+        }
+        m_Man.OpenWANTransports("zmq", adios2::OpenMode::Read, para, true);

        std::string method_type;
        int num_channels = 0;


numChannels

williamfgc · 2017-09-20T18:16:02Z

source/adios2/engine/dataman/DataManWriter.cpp

+        //            }
+        //        }
+        //        jmsg["stream_mode"] = "sender";
+        //        m_Man.add_stream(jmsg);

        int n_Transports = 1;
        std::vector<Params> para(n_Transports);


Just do std::vector<Params>parameters (1); I usually use plural names (e.g. parameters) for containers, so in a for range loop we can do something like this:

for( const auto& parameter : parameters ) { }

williamfgc · 2017-09-20T18:16:49Z

source/adios2/engine/dataman/DataManWriter.cpp

-                std::cout << j.first << " " << j.second << std::endl;
-            }
-        }
-
        m_Man.OpenWANTransports("zmq", adios2::OpenMode::Write, para, true);

        std::string method_type;


williamfgc · 2017-09-20T18:17:40Z

source/adios2/engine/dataman/DataManWriter.tcc

-//    m_Man.put(values, jmsg);
+    jmsg["bytes"] =
+        std::accumulate(variable.m_Shape.begin(), variable.m_Shape.end(),
+                        sizeof(T), std::multiplies<size_t>());


Nice use of STL numeric!

chuckatkins · 2017-09-20T19:14:56Z

source/adios2/CMakeLists.txt

+  )
+  target_link_libraries(adios2 PRIVATE ZeroMQ::ZMQ)
+endif()
+
 if(ADIOS2_HAVE_DataMan)


Is DataMan still optional? if not then the conditional check should be removed and just always add it.

chuckatkins · 2017-09-20T19:15:30Z

source/adios2/CMakeLists.txt

 if(ADIOS2_HAVE_DataMan)
  target_sources(adios2 PRIVATE
    engine/dataman/DataManReader.cpp
    engine/dataman/DataManWriter.cpp
  )
-  target_link_libraries(adios2 PRIVATE dataman)
+  target_link_libraries(adios2 PRIVATE NLohmannJson)


The build error is from the JSON library only being included if DataMan was enabled, which is off by default on Windows.

chuckatkins · 2017-09-20T19:18:45Z

source/adios2/ADIOSConfig.h.in

@@ -39,6 +39,9 @@
 /* CMake Option: ADIOS_USE_HDF5=ON */
 #cmakedefine ADIOS2_HAVE_HDF5

+/* CMake Option: ADIOS_USE_ZeroMQ=ON */
+#cmakedefine ADIOS2_HAVE_ZEROMQ
+
 /* CMake Option: ADIOS_USE_DataMan=ON */
 #cmakedefine ADIOS2_HAVE_DATAMAN


Remove if DataMan is no longer optional

chuckatkins · 2017-09-20T19:20:16Z

source/adios2/toolkit/transport/wan/WANZmq.cpp

@@ -22,7 +23,7 @@ WANZmq::WANZmq(const std::string ipAddress, const std::string port,
 : Transport("wan", "zmq", mpiComm, debugMode), m_IPAddress(ipAddress),
  m_Port(port)
 {
-
+    m_Context = zmq_ctx_new();


Does this need to be checked for validity in case it fails or will it always be guaranteed to succeed?

chuckatkins · 2017-09-20T19:22:15Z

source/adios2/toolkit/transport/wan/WANZmq.cpp

@@ -51,7 +56,7 @@ void WANZmq::Open(const std::string &name, const OpenMode openMode)

        m_Socket = zmq_socket(m_Context, ZMQ_REQ);
        const std::string fullIP("tcp://" + m_IPAddress + ":" + m_Port);
-        zmq_connect(m_Socket, fullIP.c_str());
+        int err = zmq_connect(m_Socket, fullIP.c_str());


Does err need to be checked? What happens if zmq_connect fails?

@JasonRuonanWang @chuckatkins makes a good point. Let's address this with a runtime exception check that is always on. On a side note, this is what I mean by avoiding the underscore, just by looking at zmq_connect I know it's coming from a library, it's not from us.

chuckatkins

With DataMan integrated into core adios now, it seems it's no longer optional. There are a few places that DataMan things are always added and a few that are conditionally added. It should be one or the other, and I expect it's just always on now right? That should address your json.hpp build failure.

Also a few places are missing error handling.

Other than that, looks great!

JasonRuonanWang and others added 3 commits July 31, 2017 09:47

removed original DataMan

9b3ca77

finished DataMan Transport Manager refactoring, but full functionalit…

75ded75

…y still pending on Read API

solved conflicts

30aa224

JasonRuonanWang requested review from chuckatkins and williamfgc September 19, 2017 18:33

fixed clang-format

da08bf5

JasonRuonanWang mentioned this pull request Sep 20, 2017

JSON dependency not found on Windows #271

Closed

williamfgc requested changes Sep 20, 2017

View reviewed changes

made changes as requested

04a5e5b

chuckatkins reviewed Sep 20, 2017

View reviewed changes

chuckatkins suggested changes Sep 20, 2017

View reviewed changes

JasonRuonanWang added 2 commits September 20, 2017 16:27

fixed a few error handling and solved the Windows build error

3368696

removed sleep in DataManWriter

84a1dc9

chuckatkins approved these changes Sep 21, 2017

View reviewed changes

JasonRuonanWang merged commit 57a8b1f into ornladios:master Sep 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finished DataMan Transport Manager refactoring but full functionality still pending on Read API #270

Finished DataMan Transport Manager refactoring but full functionality still pending on Read API #270

JasonRuonanWang commented Sep 19, 2017

JasonRuonanWang commented Sep 19, 2017

chuckatkins commented Sep 19, 2017

williamfgc left a comment

williamfgc Sep 20, 2017

williamfgc Sep 20, 2017

williamfgc Sep 20, 2017

williamfgc Sep 20, 2017

williamfgc Sep 20, 2017

williamfgc Sep 20, 2017

williamfgc Sep 20, 2017

williamfgc Sep 20, 2017

williamfgc Sep 20, 2017

williamfgc Sep 20, 2017

chuckatkins Sep 20, 2017

chuckatkins Sep 20, 2017

chuckatkins Sep 20, 2017

chuckatkins Sep 20, 2017

chuckatkins Sep 20, 2017

williamfgc Sep 20, 2017 •

edited

Loading

chuckatkins left a comment •

edited

Loading

Finished DataMan Transport Manager refactoring but full functionality still pending on Read API #270

Finished DataMan Transport Manager refactoring but full functionality still pending on Read API #270

Conversation

JasonRuonanWang commented Sep 19, 2017

JasonRuonanWang commented Sep 19, 2017

chuckatkins commented Sep 19, 2017

williamfgc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamfgc Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

chuckatkins left a comment • edited Loading

Choose a reason for hiding this comment

williamfgc Sep 20, 2017 •

edited

Loading

chuckatkins left a comment •

edited

Loading