[Feat/aut951] - Distributed Mode #437

ghost · 2021-10-14T17:44:44Z

Objective

SmartPeakServer App
Offload computational workloads to a dedicated entity running on HPCs as a microservice.
Modifications to SmartPeakGUI with enabled communications to the SmartPeakServer via HTTP2 to ensure lightweight operation on the client side.

…artPeak into feat/aut951B

bertrandboudaud

Niced work, please see my comments.

bertrandboudaud · 2021-12-16T13:37:39Z

src/examples/CMakeLists.txt

 cmake_minimum_required(VERSION 3.8.2 FATAL_ERROR)
 project("SmartPeak_class_examples_smartpeak")
-
+#cmake_policy(SET CMP0115 OLD)


is it still needed? if yes, can you add some comment on that

No, it was a reminder to append source code suffixes to file name, recent versions of CMake (>3.19) will produce lots of warnings when that's not the case which is annoying when debugging.

bertrandboudaud · 2021-12-16T13:39:18Z

src/examples/CMakeLists.txt

-      target_link_libraries(${_examples} PUBLIC ${SmartPeak_LIBRARIES} OpenMS)
+      target_link_libraries(${_examples} PUBLIC ${SmartPeak_LIBRARIES} OpenMS 
+						PRIVATE 
+						${grpc_required_libs})


do the examples need grpc dependency?

Yes they do.

This concerns me a bit. Can you elaborate on why the examples would need GRPC dependencies? From my understanding, none of the examples use GRPC?

bertrandboudaud · 2021-12-16T13:42:35Z

src/examples/source/SmartPeakGUI.cpp

+    // ======================================
+    // Server-Side Execution
+    // ======================================
+      if (run_workflow_widget_->server_fields_set)


would it be possible to put that block of code in a dedicated (several?) class?

I have packed some of the lines into separate functions, it's not possible to do that for the whole block since we don't want to have the gRPC library as part of the libSmartPeak or libSmartPeakWidgets.

bertrandboudaud · 2021-12-16T13:47:18Z

src/examples/source/service/services.hpp

@@ -0,0 +1,490 @@
+// --------------------------------------------------------------------------


Can we have a header and cpp file?

This file binds directly to the SmartPeakServer executable, making a library out of it is not necessary and it's not a good idea to have it as part of the main SmartPeak library.

I would also recommend a header/cpp file. If for some reason that is not possible, I would recommend using the same convention but placing what would be the .cpp contents after the .h contents.

bertrandboudaud · 2021-12-16T13:53:19Z

src/smartpeak/source/core/Server.cpp

+namespace SmartPeak {
+  namespace serv {
+
+    bool contains_option(


the 3 methods contains_option, extract_report_sampletypes and extract_report_metadata should follow the global coding standard that is start with non capital leter, then no underscore but capital letter as a separator

bertrandboudaud · 2021-12-16T13:58:26Z

protos/workflow.proto

+  rpc runWorkflow (WorkflowParameters) returns (WorkflowResult) {}
+  rpc getLogStream (InquireLogs) returns (stream LogStream) {}
+}
+


do we need all this below? report, graph data .. all this should be local there should not be any message about this, as i understood.

They will be needed later when the other approaches for transmitting the visualisation data that we have discussed is implemented.

Can you elaborate on the need for this here?

bertrandboudaud · 2021-12-16T14:06:45Z

src/examples/source/service/workflow.grpc.pb.cc

@@ -0,0 +1,247 @@
+// Generated by the gRPC C++ plugin.


As these files are generated, is it worth putting them in the repository? at first, I would say no, but I don't have a strict opinion on this. But if we put them in our repository, how is it generated? if it's generated by hand, would be nice to document in an appropriate place (maybe inline documentation in the .proto file would be a convenient location)

A short notice on how to generate them is now included in the proto file.

These files are need for the SmartPeakServer and SmartPeakGUI.

dmccloskey · 2021-12-24T16:09:21Z

protos/workflow.proto

+  rpc runWorkflow (WorkflowParameters) returns (WorkflowResult) {}
+  rpc getLogStream (InquireLogs) returns (stream LogStream) {}
+}
+


Can you elaborate on the need for this here?

src/examples/CMakeLists.txt

dmccloskey · 2021-12-24T16:10:57Z

src/examples/CMakeLists.txt

-      target_link_libraries(${_examples} PUBLIC ${SmartPeak_LIBRARIES} OpenMS)
+      target_link_libraries(${_examples} PUBLIC ${SmartPeak_LIBRARIES} OpenMS 
+						PRIVATE 
+						${grpc_required_libs})


This concerns me a bit. Can you elaborate on why the examples would need GRPC dependencies? From my understanding, none of the examples use GRPC?

dmccloskey · 2021-12-24T16:13:59Z

src/examples/source/service/services.hpp

@@ -0,0 +1,490 @@
+// --------------------------------------------------------------------------


I would also recommend a header/cpp file. If for some reason that is not possible, I would recommend using the same convention but placing what would be the .cpp contents after the .h contents.

dmccloskey

The updates look really nice. In addition, looks like the unit tests for the new Utility methods are missing? If so can you add them as well.

ghost · 2021-12-30T14:54:57Z

The updates look really nice. In addition, looks like the unit tests for the new Utility methods are missing? If so can you add them as well.

Thanks for reminding, I have added the unittests for the new utility methods.

dmccloskey

Thanks for adding in the unit tests. This is looking good from my end. Let's wait on Bertrands feedback and then go from there.

bertrandboudaud

ok for me

Ahmed Khalil added 18 commits September 10, 2021 17:58

[SERV] - initial setup

17d50b2

[SERV] - sync/async impl. on client-/server-side

f8c2fe1

[SERV] - chromatogram plotting support for client

18ff6ac

[SERV] - heatmap plotting beta

cb4672b

[SERV] - SmartPeakServer v1.0.0

c7b7f87

[SERV] - SmartPeakServer v1.0

2815073

concluding merging

ca25cf6

typo fix

22d4831

unset cmake policy

bd32a70

minor fix

70ffa2d

minor changes

23c72ab

[SERV] - providing URLs as args

99783c0

[TEST] - updating tests

4f3c58b

minor changes

79ce76c

minor changes

a50c00f

fixing build issues on macOS

326ef98

fixing OpenMS build

5342c11

[PACK] - fix win packaging

7b8ad5f

ghost changed the title ~~[Feat/aut951] - SmartPeakServer v1.0.0~~ [Feat/aut951] - Distributed Mode Dec 10, 2021

Ahmed Khalil added 11 commits December 10, 2021 14:15

[MERGE] merging develop into feat/aut951B

57cb324

cherrypicking v1.17.0->feat/aut951B

41a7806

[DOC,TEST] - licence notice & testing fix

9a7e0da

[TEST] - fixing gtest conflicts

1ca94a5

[BUILD] - win fix

94a83da

libssl integration

57cce7a

[BUILD] - build fixes

778d1b9

[BUILD] - build fixes

d9b31a3

[BUILD] - minor fix

16f8732

[BUILD] - minor changes

b8fdc47

[skip ci][TOOLS] - adding smartpeak user mgmt tools

157bb3f

Ahmed Khalil added 2 commits December 15, 2021 20:42

Merge branch 'feat/aut951B' of https://github.com/AutoFlowResearch/Sm…

2f23a8f

…artPeak into feat/aut951B

[DOC] - adding docs

b76d761

ghost requested review from bertrandboudaud and dmccloskey December 16, 2021 10:29

bertrandboudaud requested changes Dec 16, 2021

View reviewed changes

Ahmed Khalil added 5 commits December 17, 2021 12:07

[DOC] - minor changes[skip ci]

088f36e

[API] - minor updates

68c9ac7

[API] - minor updates

3ba2482

[API] - minor updates

9d79905

[API] - update to newer OpenMS RangeManager API

b4717a3

dmccloskey reviewed Dec 24, 2021

View reviewed changes

Ahmed Khalil added 3 commits December 27, 2021 14:38

[API] - minor changes

c6280b3

minor changes/additions

5cf93a0

[DOC][skip ci] - minor update

5b44def

dmccloskey reviewed Dec 30, 2021

View reviewed changes

[TEST] - util. unittests

418543d

Ahmed Khalil added 2 commits January 4, 2022 13:55

[DOC][skip ci] - info msg

5458ca0

[DOC][skip ci] - debug msg

a61fdce

dmccloskey reviewed Jan 6, 2022

View reviewed changes

bertrandboudaud approved these changes Jan 6, 2022

View reviewed changes

bertrandboudaud merged commit c0507d4 into develop Jan 6, 2022

		@@ -0,0 +1,490 @@
		// --------------------------------------------------------------------------

[Feat/aut951] - Distributed Mode #437

[Feat/aut951] - Distributed Mode #437

Uh oh!

Conversation

ghost commented Oct 14, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bertrandboudaud 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmccloskey left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Dec 30, 2021

Uh oh!

dmccloskey left a comment

Choose a reason for hiding this comment

Uh oh!

bertrandboudaud left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ghost commented Oct 14, 2021 •

edited by ghost

Loading