zeroeq::Server/Client based morphology loader #172

eile · 2017-08-04T07:43:04Z

No description provided.

eile · 2017-08-04T12:45:34Z

Retest this please.

eile · 2017-08-07T12:03:24Z

ping?

rdumusc · 2017-08-07T11:56:08Z

brain/CMakeLists.txt

 )
 set(BRAIN_INCLUDE_NAME brain)
 set(BRAIN_NAMESPACE brain)

 if(TARGET MVDTool)
-  list(APPEND BRAIN_LINK_LIBRARIES PRIVATE MVDTool)
+  list(APPEND BRAIN_LINK_LIBRARIES MVDTool)


Now I don't know if you're trying to make it PUBLIC or relying on the context to keep it PRIVATE. I would recommend leaving the keyword for readability.

rdumusc · 2017-08-07T12:19:32Z

brion/plugin/morphologyZeroEQ.h

+{
+namespace plugin
+{
+class MorphologyZeroEQ : public MorphologyPlugin, public servus::Serializable


I'm confused at what this class represents. Why would anyone want to serialize a plugin? Especially when the base class is boost::noncopyable? It needs at the very least some documentation, possibly refactoring.

Let me see what I can do. It's both a serializable morphology (for brain & ZeroEQ) and the ZeroEQ plugin.

rdumusc · 2017-08-07T12:51:23Z

brain/neuron/morphologyImpl.cpp

@@ -76,47 +47,20 @@ Morphology::Impl::Impl(const brion::Morphology& morphology)
    somaSection = ids[0];
 }

-bool Morphology::Impl::_fromBinary(const void* data, const size_t size)
+Morphology::Impl::Impl(const brion::Morphology& morphology)
+    : brion::plugin::MorphologyZeroEQ(morphology)


Seriously, we have a brain::Morphology::Impl::Impl() which take a brion::Morphology& (which is itself a wrapper around a brion::MorphologyPlugin) and yet manges to derive from brion::plugin::MorphologyZeroEQ (a brion::MorphologyPlugin subclass) itself? I think that fuels my confusion below...

^ see above. Likely I'll move the serializable morphology to a helper class.

This inheritance doesn't neither look right to me at all.
First of all, ZeroEQ is an optional dependency. Given that, making a core class of Brain depend on something that has the ZeroEQ suffix doesn't only smell fishy, is already rotten. Maybe it's just the naming because I haven't reached the class declaration yet, but it could also be that inheritance is used where it should be aggregation.

rdumusc · 2017-08-07T13:47:01Z

tests/perf/circuit.cpp

+BOOST_AUTO_TEST_CASE(load_morphologies)
+{
+    const brain::Circuit circuit(
+        (brion::URI("/home/eilemann/stable/Brion/BlueConfig")));


home folder path? What happens if someone runs ninja Brion-perftests?

Accidental testing artefact. Fixed.

rdumusc · 2017-08-07T13:52:06Z

tests/perf/circuit.cpp

+    const brain::Circuit circuit(
+        (brion::URI("/home/eilemann/stable/Brion/BlueConfig")));
+    auto gids = circuit.getGIDs();
+    if (gids.size() > 50000)


make this a function, something like:
resizeIfBiggerThan(gids, 50000);

hernando · 2017-08-07T14:53:03Z

apps/CMakeLists.txt

@@ -24,6 +24,13 @@ if(TARGET BBPTestData)
  list(APPEND SPIKECONVERTER_LINK_LIBRARIES BBPTestData)
 endif()

+if(TARGET ZeroEQ)
+  set(MORPHOLOGYSERVER_HEADERS)


Remove if empty.

hernando · 2017-08-07T14:56:06Z

apps/CMakeLists.txt

@@ -24,6 +24,13 @@ if(TARGET BBPTestData)
  list(APPEND SPIKECONVERTER_LINK_LIBRARIES BBPTestData)
 endif()

+if(TARGET ZeroEQ)


Keyv is also optional

common_find_package(Keyv REQUIRED) ?

Where did I looked at then...? I checked it before commenting.

hernando · 2017-08-07T15:04:15Z

apps/morphologyServer.cpp

+            if (value.ptr && value.size)
+            {
+                std::cout << 'c' << std::flush;
+                return zeroeq::ReplyData(brion::ZEROEQ_GET_MORPHOLOGY, value);


Maybe it's a bit late to add this comment here, but to me it seems wrong to require the request ID in the return value of this function. If the handler doesn't take any parameters to distinguish the request ID, it means that you can't bind the same function to multiple handlers, which in turns implies that the request ID is implicit. What's the reason for not returning a Serializable and let ZeroEQ figure out the right request ID to use (you could always return an empty answer returning by unique_ptr.

What does it happen is the user changes the request ID to something else?

It returns the reply ID, not the request ID (which in this use case is identical to the request ID). A handler may return different reply IDs to the same request, e.g., for error codes or different return value types.

That was not obvious by the code because in case of errors you're returning and empty reply.

hernando · 2017-08-07T15:06:26Z

apps/morphologyServer.cpp

+        << "  zeroeq://" << address << "/path/to/morphology" << std::endl
+        << std::endl
+        << "  [c]ache read, [d]isk read with cache update, disk read "
+           "with cache [e]rror, [D]isk read uncached, morphology [l]oad error: "


I like u better that D for disk read uncached. So can rewrite it as [u]ncached disk read

hernando · 2017-08-07T15:08:47Z

apps/morphologyServer.cpp

+#include <brion/morphology.h>
+#include <brion/plugin/morphologyZeroEQ.h>
+#include <keyv/Map.h>
+#include <lunchbox/daemon.h>


hernando · 2017-08-08T09:05:22Z

brain/circuit.cpp

+            brion::Morphology& raw = loading.find(hash)->second;
+            neuron::MorphologyPtr morphology(
+                transform ? new neuron::Morphology(raw, transforms[i])
+                          : new neuron::Morphology(raw));


You're not updating cached anymore, well, yes, with a nullptr above.

good catch. Fixed.

hernando · 2017-08-08T09:12:44Z

brain/neuron/morphologyImpl.cpp

@@ -76,47 +47,20 @@ Morphology::Impl::Impl(const brion::Morphology& morphology)
    somaSection = ids[0];
 }

-bool Morphology::Impl::_fromBinary(const void* data, const size_t size)
+Morphology::Impl::Impl(const brion::Morphology& morphology)
+    : brion::plugin::MorphologyZeroEQ(morphology)


This inheritance doesn't neither look right to me at all.
First of all, ZeroEQ is an optional dependency. Given that, making a core class of Brain depend on something that has the ZeroEQ suffix doesn't only smell fishy, is already rotten. Maybe it's just the naming because I haven't reached the class declaration yet, but it could also be that inheritance is used where it should be aggregation.

hernando · 2017-08-08T09:24:42Z

brion/morphologyPlugin.h

-        , version(MORPHOLOGY_VERSION_H5_1_1)
-        , family(FAMILY_NEURON)
+        , version(v)
+        , family(f)


This should throw when CellFamiliy is FAMILY_GLIA and the version is not 1.1. The old code was a bit strange exactly to prevent that from being possible at all.

hernando · 2017-08-08T09:27:23Z

brion/plugin/CMakeLists.txt

@@ -26,6 +27,7 @@ set(BRIONPLUGINS_SOURCES
  compartmentReportMap.cpp
  morphologyHDF5.cpp
  morphologySWC.cpp
+  morphologyZeroEQ.cpp


The ZeroEQ sources should be added conditionally

Now yes, after the serialization code is factored out. Done.

hernando · 2017-08-08T09:33:18Z

tests/morphology.cpp

+        const std::string path((const char*)data, size);
+        const brion::Morphology morphology(path);
+        const brion::plugin::MorphologyZeroEQ serializable{morphology};
+        return zeroeq::ReplyData(serializable.getTypeIdentifier(),


I'm confused by this getTypeIdentifier(), it seems that it wouldn't match the code in the server application, where you use ZEROEQ_GET_MORPHOLOGY.

It doesn't match, but since it's ignored by the reply handler it does not matter. Fixed.

eile · 2017-08-09T09:39:01Z

@rdumusc all: See last commit for a substantial cleanup in the internals. This breaks the API - shall I bump major?

eile · 2017-08-24T07:48:00Z

Ping^2

tribal-tec

Version bump to major 3?

eile · 2017-08-24T08:13:34Z

Version bump to major 3?

done

tribal-tec · 2017-08-24T08:29:35Z

brion/morphology.cpp

+        if (!loadFuture.valid())
+            return;
+
+        loadFuture.wait();


eile force-pushed the zeromorph branch 5 times, most recently from 7effdd2 to 00cb7b3 Compare August 4, 2017 07:51

eile requested review from hernando, tribal-tec and rdumusc and removed request for hernando August 4, 2017 07:51

eile force-pushed the zeromorph branch from d4cdc12 to eaca4b4 Compare August 4, 2017 08:45

eile force-pushed the zeromorph branch 3 times, most recently from 12d03eb to e9e2cb6 Compare August 7, 2017 09:11

rdumusc reviewed Aug 7, 2017

View reviewed changes

hernando reviewed Aug 8, 2017

View reviewed changes

eile force-pushed the zeromorph branch from d50479b to 3a77efd Compare August 9, 2017 09:38

eile pushed a commit to eile/Brion that referenced this pull request Aug 9, 2017

CR BlueBrain#172

48f1bce

eile force-pushed the zeromorph branch from 48f1bce to c8a0ff3 Compare August 9, 2017 09:41

eile pushed a commit to eile/Brion that referenced this pull request Aug 9, 2017

CR BlueBrain#172

c8a0ff3

eile force-pushed the zeromorph branch from c8a0ff3 to a5d94fb Compare August 9, 2017 09:42

eile pushed a commit to eile/Brion that referenced this pull request Aug 9, 2017

CR BlueBrain#172

a5d94fb

eile force-pushed the zeromorph branch from a5d94fb to a15b01c Compare August 9, 2017 09:46

eile pushed a commit to eile/Brion that referenced this pull request Aug 9, 2017

CR BlueBrain#172

a15b01c

eile force-pushed the zeromorph branch from a15b01c to ef81b5d Compare August 9, 2017 09:50

eile pushed a commit to eile/Brion that referenced this pull request Aug 9, 2017

CR BlueBrain#172

ef81b5d

eile force-pushed the zeromorph branch from ef81b5d to 118143a Compare August 9, 2017 09:52

eile force-pushed the zeromorph branch 6 times, most recently from 2b1bbf1 to f2f8f49 Compare August 23, 2017 13:53

hernando mentioned this pull request Aug 23, 2017

HDF5++ replaced with HighFive #176

Merged

Stefan Eilemann added 10 commits August 24, 2017 09:48

zeroeq::Server/Client based morphology loader

5fc61ce

Async loader

140d241

Bump memcached keys since serialization changed

25f0020

Clean up morphology API and internal hierarchy for serialization

94b87d4

CR BlueBrain#172

6d7ad9d

Document reasoning for new Morphology API

cc1a38f

Remove Morphology write API

991659a

New read API (see doc/feature/morphologyRead.md)

36520ba

Remove ZeroEQ from subprojects

5535665

Increase test coverage & fixes

5e5aecb

eile force-pushed the zeromorph branch from f2f8f49 to 5e5aecb Compare August 24, 2017 07:48

tribal-tec approved these changes Aug 24, 2017

View reviewed changes

eile pushed a commit to eile/Brion that referenced this pull request Aug 24, 2017

CR BlueBrain#172

980b58f

tribal-tec reviewed Aug 24, 2017

View reviewed changes

brion/morphology.cpp Outdated

if (!loadFuture.valid())

return;

loadFuture.wait();

Copy link

Contributor

tribal-tec Aug 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rm

eile force-pushed the zeromorph branch from 980b58f to a3efdbb Compare August 24, 2017 09:06

eile pushed a commit to eile/Brion that referenced this pull request Aug 24, 2017

CR BlueBrain#172

a3efdbb

CR BlueBrain#172

0cdac4d

eile force-pushed the zeromorph branch from a3efdbb to 0cdac4d Compare August 24, 2017 09:16

eile merged commit 75a42bc into BlueBrain:master Aug 24, 2017

eile deleted the zeromorph branch August 24, 2017 12:26

zeroeq::Server/Client based morphology loader #172

zeroeq::Server/Client based morphology loader #172

Conversation

eile commented Aug 4, 2017

eile commented Aug 4, 2017

eile commented Aug 7, 2017

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

eile Aug 8, 2017 • edited

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

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

eile commented Aug 9, 2017 • edited

eile commented Aug 24, 2017

tribal-tec left a comment

Choose a reason for hiding this comment

eile commented Aug 24, 2017 • edited

Choose a reason for hiding this comment

eile Aug 8, 2017 •

edited

eile commented Aug 9, 2017 •

edited

eile commented Aug 24, 2017 •

edited