Loaders & CircuitExplorer refactoring #927

NadirRoGue · 2021-10-27T15:47:17Z

Refactor of loaders to use compile time parameters
Update copyright year on source files
Refactor CircuitExplorer
Adds support for SONATA format

…ice year

Adrien4193 · 2021-10-29T07:03:38Z

brayns/common/loader/Loader.h

+    Loader(Scene& scene)
+        : _scene(scene)
+    {
+    }


Just a personal opinion but for me it is a real pain to have non-default constructors in base classes as you need to rewrite it for each subclasses...
It would be easier to decouple initialization and construction like that:

class Loader { public: virtual ~Loader() = default; virtual void onCreate() {} void setScene(Scene &scene) {_scene = &scene;} Scene &getScene() const {assert(_scene); return *_scene;} private: Scene *_scene = nullptr; }; // On register: loader->setScene(scene); loader->onCreate();

The point of the design is to comply with RAII so that you cannot use a loader without the scene being set up.
Otherwise someone could instantiate the loader without know he must call the setScene method.

Yes but if they are meant to be registered as a list of abstract objects that will be used in an agnostic fashion using the extension of the files to load, it is not really a problem.

But if you prefer like that it's fine also.

They can always be used manually in an entry point, for instance, when you want to load an specific type of model and do something with it. And then again, doing it with the setScene method breask RAII.

I believe this way is more correct, whereas setting it afterwards may be dangerous.

Adrien4193 · 2021-10-29T07:05:07Z

brayns/common/loader/Loader.h

+ * @brief The EmptyLoaderParameters struct is a convenience type for loaders
+ * that do not have any input parameter
+ */
+struct EmptyLoaderParameters


There is an EmptyMessage already existing in MessageFactory.h, I don't know if you could have used it...

MessageFactory is part of the network library, which does not make sense to link against braynsCommon.

We could extract it to brayns/json as it is just an empty struct (don't even need to be a message I realize) with an adapter.

But it can be done in a later pass.

We will see. We have to refactor braynsJson, since it does not make sense its called MESSAGE_... (thats semantically correct for network though). I believe we should call the macros something like SERIALIZABLE_.... We can introduce the empty serializable struct on braynsJson in that refactor and remove this struct here.

Adrien4193 · 2021-10-29T07:26:25Z

brayns/common/loader/Loader.h

+        const JsonValue& params) const override
+    {
+        T inputParams;
+        if (!Json::deserialize<T>(params, inputParams))


You can use JsonValidator::validate if you want to check all constraints in the JSON schema and have a more detailed error and throw the list of errors in the JSON-RPC error.data field using EntrypointException.

But depending on how you use it, it might already be done in the entrypoint so in this case you don't even need to check the return value.

The entry point just copy the json object that contains the parameter, so Ill add the validation. Thanks for the suggestion!

Adrien4193 · 2021-10-29T08:01:59Z

brayns/io/ProteinLoaderParameters.h

+    {"protein_residues", ProteinLoaderColorScheme::protein_residues});
+
+BRAYNS_MESSAGE_BEGIN(ProteinLoaderParameters)
+BRAYNS_MESSAGE_ENTRY_DEFAULT(ProteinLoaderColorScheme, color_scheme,


You can already specify a default value using Default(yourJsonValue) in the options after description, the advantage of doing so is that the default value will be specified in the JSON schema.

Ah, I didnt know. Ill remove the macro I added and use that. It would be something like: BRAYNS_MESSAGE_ENTRY(ProteinLoaderColorScheme, color_scheme, ProteinLoaderColorScheme::none, "Descriptrion") ?

Nope something like:
BRAYNS_MESSAGE_ENTRY(Type, name, "description", Default("none"));

This is a JSON stuff so if you just want something to initialize the struct then you stuff is fine but if you need a JSON default value if this one is missing, then it is better to use the option as it will be marked in the schema.

Adrien4193 · 2021-10-29T08:03:43Z

brayns/json/CMakeLists.txt

@@ -0,0 +1,55 @@
+#cmake_minimum_required(VERSION 3.15 FATAL_ERROR)


Why are all these commands commented ? In prevision of the removal of CMake/common ?

All of brayns core libraries are only compiled as part of the brayns project. It does not make sense to keep checking for version when we already do in the main CMakeListst.txt.

We can remove the comments then ?

Adrien4193 · 2021-10-29T08:06:29Z

brayns/json/Message.h

@@ -252,7 +245,7 @@ class MessageInfo
 * will be serialized using JsonAdapter<TYPE>.
 *
 */
-#define BRAYNS_MESSAGE_PROPERTY(TYPE, NAME, ...)                              \
+#define BRAYNS_MESSAGE_PROPERTY(TYPE, NAME, DEFAULT, ...)                     \


The Default(json) value allows to set a default value that will be visible in the JSON schema so if the property is not found, it will make as if the provided JSON was extracted.

But if you just need to set a default value programatically in the struct without specifying it in the JSON schema, then I guess it is fine.

Yes, I didnt know it was possible already. Ill change it back.

Adrien4193 · 2021-10-29T08:35:45Z

plugins/CircuitExplorer/plugin/api/CircuitColorHandler.cpp

+
+namespace
+{
+inline std::vector<uint64_t> __parseIDRanges(const std::string& input)


Why inline ?

Double underscores names are reserved for the compiler no ?

I didnt know double underscores where reserverd for the compiler. I know thats "kind of" a rule for python private vars, but I've seen this type of coding style in other softward. I personally use double underscore to denote anonymous private namespace functions.

I mark it inline because its a function used only within the cpp where is defined (usually just once or twice max), so I though I would avoid an extra jump without compiled code storage space overhead.

Adrien4193 · 2021-10-29T08:37:54Z

plugins/CircuitExplorer/plugin/api/FrameExportManager.cpp

+
+namespace
+{
+inline auto __getFIImageFormat(const std::string& format) noexcept


I mark it inline because its a function used only within the cpp where is defined (usually just once or twice max), so I though I would avoid an extra jump without compiled code storage space overhead.

Adrien4193 · 2021-10-29T08:40:44Z

plugins/CircuitExplorer/plugin/io/BBPLoader.cpp

+
+namespace
+{
+inline std::unique_ptr<Simulation> __intantiateSimulation(


I mark it inline because its a function used only within the cpp where is defined (usually just once or twice max), so I though I would avoid an extra jump without compiled code storage space overhead.

Adrien4193 · 2021-10-29T08:41:53Z

plugins/CircuitExplorer/plugin/io/NeuronMorphologyLoader.cpp

+
+namespace
+{
+inline void __checkInput(const NeuronMorphologyLoaderParameters& input)


I mark it inline because its a function used only within the cpp where is defined (usually just once or twice max), so I though I would avoid an extra jump without compiled code storage space overhead.

Adrien4193 · 2021-10-29T08:43:06Z

plugins/CircuitExplorer/plugin/io/SonataLoader.cpp

-    _fixedDefaults.add(
-        {PROP_POSTSYNAPTIC_NEURON_GID.getName(), std::string("")});
-    _fixedDefaults.add(PROP_SYNCHRONOUS_MODE);
+inline auto selectNodes(const bbp::sonata::CircuitConfig& config,


I mark it inline because its a function used only within the cpp where is defined (usually just once or twice max), so I though I would avoid an extra jump without compiled code storage space overhead.

Adrien4193 · 2021-10-29T08:45:31Z

plugins/CircuitExplorer/plugin/io/bbploader/SynapseLoader.cpp

+{
+namespace
+{
+inline auto __load(const brain::Synapses& src, const brain::GIDSet& gids,


I mark it inline because its a function used only within the cpp where is defined (usually just once or twice max), so I though I would avoid an extra jump without compiled code storage space overhead.

Adrien4193 · 2021-10-29T08:47:41Z

plugins/CircuitExplorer/plugin/io/bbploader/simulation/handlers/CompartmentHandler.cpp

+
+namespace
+{
+inline auto __frameIndexToTimestamp(const uint32_t frame,


I mark it inline because its a function used only within the cpp where is defined (usually just once or twice max), so I though I would avoid an extra jump without compiled code storage space overhead.

Adrien4193 · 2021-10-29T08:53:12Z

plugins/CircuitExplorer/plugin/io/morphology/neuron/pipeline/SectionWelder.cpp

+};
+} // namespace
+
+void SectionWelder::proccess(NeuronMorphology& morphology) const


Ill fix it. The type is also present in 3 other classes

Adrien4193 · 2021-10-29T08:55:59Z

plugins/CircuitExplorer/plugin/io/sonataloader/data/SonataCells.cpp

+constexpr char attribEtype[] = "etype";
+constexpr char attribMorphology[] = "morphology";
+
+inline std::vector<std::string> __getEnumValueList(


I mark it inline because its a function used only within the cpp where is defined (usually just once or twice max), so I though I would avoid an extra jump without compiled code storage space overhead.

Adrien4193 · 2021-10-29T08:57:26Z

plugins/CircuitExplorer/plugin/io/sonataloader/data/SonataSimulationMapping.cpp

+{
+using Range = std::pair<uint64_t, uint64_t>;
+
+inline auto __computeMapping(


I mark it inline because its a function used only within the cpp where is defined (usually just once or twice max), so I though I would avoid an extra jump without compiled code storage space overhead.

Adrien4193 · 2021-10-29T09:02:57Z

plugins/CircuitExplorer/plugin/network/adapters/ColoringInfoAdapter.h

+{
+BRAYNS_ADAPTER_BEGIN(ColoringInformation)
+BRAYNS_ADAPTER_NAMED_ENTRY("variable", variable,


You can use BRAYNS_ADAPTER_ENTRY directly as the name is the same as the JSON-RPC one.

Yeah, ill change it

Adrien4193 · 2021-10-29T09:10:07Z

plugins/CircuitExplorer/plugin/network/messages/ExportFramesToDiskMessage.h

+BRAYNS_MESSAGE_ENTRY(
+    uint32_t, spp,
+    "Samples per pixels (The more, the better visual result and "
+    "the slower the"


clang automatic string slicing doesn't put them together it seems ^^

Adrien4193

I left some minor comments but nothing blocking, if you want to merge like that it is OK for me.

- Using already in place default values for json messages - Fixed loader parsing empty parameters (Thanks Adrien) - Removed custom added BRAYNS_MESSAGE_ENTRY_DEFAULT - Removed unused comments in braynsJson/CMakeLists.txt - Removed inline specifier from functions - Removed double underscores __ from functions - Use JsonSchemaValidator::validate for a better error messaging when failing to parse loader parameters

NadirRoGue · 2021-10-29T14:26:45Z

I addressed some of the review comments in the last commit.
Thanks for the review and the help!

NadirRoGue · 2021-10-29T15:08:05Z

retest this please

…eserialize()

NadirRoGue · 2021-10-29T16:01:08Z

retest this please

NadirRoGue · 2021-10-29T16:20:03Z

retest this please

…all to deserialize()" This reverts commit 6309d2d.

Will avoid compiler warning/error of ambiguous call caused by implicit construction of JsonValue (Poco::Dynamic::Var)

NadirRoGue · 2021-11-01T13:14:22Z

retest this please

- Scene is not a parameter needed at loader construction time, is passed as argument when import is done - Adjusted required and default parameter values for CircuitExplorer loaders - Removed unused and inimplemented method of LoaderRegistry

* Core updates: - Extracted Json functionality from braynsNetwork into a separate library - Extracted common/utils functionality from braynsCommon into a separate library - Updates loaders to use compile time parameters. - Scene is not a parameter needed at loaders construction time, is passed as argument when import is done - Removed unused and unimplemented method of LoaderRegistry - Updates copyright notice year - Fixes simulation colors on snapshot * CircuitExplorer updates: - Rewritten frame export functionality - Removed unused loaders and rewritten remaining ones - Added full SONATA multi-population support - Making MaterialRangeProxy::commit() const to avoid ambigous call to deserialize() - Removes unnecessary cmake directives

* Bumped Pillow and websockets version. (#922) * Code cleanup (#923) * Brayns 278 add mock requests in python tests (#924) * Brayns 285 fix logic issue in quanta jpeg stream (#925) * Brayns 286 fix python image method (#928) * Brayns 287 fix python entrypoint methods with one of schema (#929) * Loaders & CircuitExplorer refactoring (#927) * Fixed some typo's (#930) * Update AUTHORS.txt (#931) * Brayns 258 create a proper logging system for brayns (#932) * Frame export improvement and static global objects removal: (#933) * Removes CMake/common dependency from build system (#934) * Removed VRPN plugin and deps. (#935) * Brayns 295 rename brayns json macros to a more appropriate name (#936) * Fixes segfault due to global const std::strings (#939) * Map and unmap framebuffer added. (#940) * Fix entry points variable naming (#941) * Brayns 300 replace free image (#942) * Brayns 308 Code restructure (#943) * Brayns 315 2.0.0 release preparation (#944)

NadirRoGue added 3 commits October 27, 2021 17:45

Updates loaders to use compile time parameters. Updates copyright not…

5aa6c61

…ice year

Bug fixing, test fix and coding formatting

a900c3e

Further clang format fixes

24a666a

NadirRoGue requested a review from Adrien4193 October 28, 2021 14:40

Adrien4193 reviewed Oct 29, 2021

View reviewed changes

Adrien4193 previously approved these changes Oct 29, 2021

View reviewed changes

NadirRoGue dismissed Adrien4193’s stale review via c3bd025 October 29, 2021 14:25

Making MaterialRangeProxy::commit() const to avoid ambigous call to d…

6309d2d

…eserialize()

NadirRoGue added 3 commits November 1, 2021 09:24

Revert "Making MaterialRangeProxy::commit() const to avoid ambigous c…

babeec3

…all to deserialize()" This reverts commit 6309d2d.

Removes unnecessary cmake directives

28da0f0

Re-added const modifier to MaterialRangeProxy::commit()

59aa735

Will avoid compiler warning/error of ambiguous call caused by implicit construction of JsonValue (Poco::Dynamic::Var)

Loader improvements:

8902a5e

- Scene is not a parameter needed at loader construction time, is passed as argument when import is done - Adjusted required and default parameter values for CircuitExplorer loaders - Removed unused and inimplemented method of LoaderRegistry

NadirRoGue force-pushed the circuitexplorer_refactor branch from 8902a5e to 24a666a Compare November 1, 2021 14:58

Try running CI with 1 thread

a205556

Adrien4193 approved these changes Nov 2, 2021

View reviewed changes

NadirRoGue merged commit 3c517e7 into develop Nov 2, 2021

NadirRoGue deleted the circuitexplorer_refactor branch November 2, 2021 08:00

		@@ -0,0 +1,55 @@
		#cmake_minimum_required(VERSION 3.15 FATAL_ERROR)

Loaders & CircuitExplorer refactoring #927

Loaders & CircuitExplorer refactoring #927

Conversation

NadirRoGue commented Oct 27, 2021

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

Adrien4193 Oct 29, 2021 • edited Loading

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

Adrien4193 left a comment

Choose a reason for hiding this comment

NadirRoGue commented Oct 29, 2021

NadirRoGue commented Oct 29, 2021

NadirRoGue commented Oct 29, 2021

NadirRoGue commented Oct 29, 2021

NadirRoGue commented Nov 1, 2021

Adrien4193 Oct 29, 2021 •

edited

Loading