Added plugins feature that allows separate binaries as processes #58

dahvid · 2020-09-22T12:03:38Z

Used the loader.go which was not compiled, I had to brutalize it a bit, to get it to work with plugins.
Need a conversation about what the intention was of some of the non-working code in it.

Used the loader which was compiled out, and had to brutilize it a bit Need a conversation about what the intention was of some of the non-working code i it

The first layer of management is that parameters can be passed to the Plugins via the Json graph, under an optional Parameters field

trustmaster

Hello @dahvid ! Thanks for your Pull Request and sorry for a late reply.

If I understood correctly, Plugins are Components that can be compiled separately from the application and loaded at run-time from shared libraries. I think it's a great contribution!

This PR needs further work to be merged to the main repo:

It would be nice to add some explanation in the description of this PR on what "plugins" are, when and how to use them.
Could you base it on master branch rather than parser? parser is WIP and it doesn't seem like these changes really depend on it.
Could you please make sure the tests pass and that test coverage remains at ~90% level?
Please add descriptive godoc strings to all exported types and functions.

See more comments inline.

trustmaster · 2020-10-10T12:16:26Z

graph.go

@@ -89,6 +89,11 @@ func (n *Graph) Add(name string, c interface{}) error {
 return nil
 }

+// Get a component by name
+func (n *Graph) Get(name string) interface{} {


This name is ambiguous. I'd suggest calling it GetProcess() instead.

trustmaster · 2020-10-10T12:17:24Z

loader.go

 // Parse JSON into Go struct
 var descr graphDescription
 err := json.Unmarshal(js, &descr)
 if err != nil {
+ fmt.Println("Error parsing JSON", err)


Please clean up all debug prints before merging, and make use of error to return errors to caller.

trustmaster · 2020-10-10T12:28:39Z

plugins.go

+const plugInSuffix = `_goplug.so`
+
+//PlugIn something
+type PlugIn interface {


Are params mandatory for all plugins? Can't we just load any components as plugins? Forcing all plugin components adhere to this interface is quite restrictive.

Also, I have a strong feeling that Parameters is reinventing IIPs. In FBP, IIPs (initial IPs) are used to send pre-defined input to components. See graph_iip_test.go for example.

trustmaster · 2020-10-10T12:30:50Z

make_plugs.sh

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


I believe this file should live in test_plugins/.

dahvid · 2020-10-12T07:02:47Z

Hi Vladimir, Yes, of course, I sort of pushed it just to see if there was anybody alive there! For the use case in my tests IIP's would work, but in the real world there are all kinds of configuration information you want to pass a component (I've built several commercial and research systems like this). It seems awkward to have to create a special input channel that will only receive data once. If I remember correctly the master branch didn't have a function for loading a graph from JSON, which is essential for my current use case for goflow. There are a few other problems with my request. 1. Goflow plugins are partly broken. They have a weird hash function in plugin.go that checks, something, not clear what. It's supposed to check if the plugin was compiled with the same version as the current go, but doesn't. It seems to me to reject plugins more or less randomly. I had to comment out the hash check in th plugin.go, to get it to work consistently. Most people have given up on .so plugins because of this. I also think it might be better to just allow compiled in plugins like you had at first. 2. I'm trying to move to a more managed plugin model than just Process(). Maybe this should be built outside of goflow? I'll try to fix some of these issues this week. But probably we need to have a conversation about your goals and I'll understand better what should be inside or outside the main project. Regards, David

…

On Sat, Oct 10, 2020 at 3:38 PM Vladimir Sibirov ***@***.***> wrote: ***@***.**** requested changes on this pull request. Hello @dahvid <https://github.com/dahvid> ! Thanks for your Pull Request and sorry for a late reply. If I understood correctly, Plugins are Components that can be compiled separately from the application and loaded at run-time from shared libraries. I think it's a great contribution! This PR needs further work to be merged to the main repo: 1. It would be nice to add some explanation in the description of this PR on what "plugins" are, when and how to use them. 2. Could you base it on master branch rather than parser? parser is WIP and it doesn't seem like these changes really depend on it. 3. Could you please make sure the tests pass and that test coverage remains at ~90% level? 4. Please add descriptive godoc strings to all exported types and functions. See more comments inline. ------------------------------ In graph.go <#58 (comment)>: > @@ -89,6 +89,11 @@ func (n *Graph) Add(name string, c interface{}) error { return nil } +// Get a component by name +func (n *Graph) Get(name string) interface{} { This name is ambiguous. I'd suggest calling it GetProcess() instead. ------------------------------ In loader.go <#58 (comment)>: > // Parse JSON into Go struct var descr graphDescription err := json.Unmarshal(js, &descr) if err != nil { + fmt.Println("Error parsing JSON", err) Please clean up all debug prints before merging, and make use of error to return errors to caller. ------------------------------ In plugins.go <#58 (comment)>: > @@ -0,0 +1,78 @@ +package goflow + +import ( + "fmt" + "io/ioutil" + "path" + "plugin" + "strings" +) + +const plugInSuffix = `_goplug.so` + +//PlugIn something +type PlugIn interface { Are params mandatory for all plugins? Can't we just load *any* components as plugins? Forcing all plugin components adhere to this interface is quite restrictive. Also, I have a strong feeling that Parameters is reinventing IIPs. In FBP, IIPs (initial IPs) are used to send pre-defined input to components. See *graph_iip_test.go* for example. ------------------------------ In make_plugs.sh <#58 (comment)>: > @@ -0,0 +1,5 @@ +#!/bin/bash I believe this file should live in test_plugins/. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#58 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABWSOM7MG6Y62MH65W3M2FDSKBIULANCNFSM4RVV5Y3Q> .

trustmaster · 2020-10-12T10:17:39Z

@dahvid

Re: IIPs vs. `map[string]interface{}` params

I agree that there are cases for more complex configuration of the components rather than just a plain scalar value. However, I believe that an Initial IP is a concept good enough to cover this problem. For example, as you're primarily dealing with JSON graph definitions, it is quite natural to have IIPs coming as JSON objects.

In the current version of JSON graph definitions, which is compatible with noflo, you could do it this way:

"connections": [
  {
    "data": {
      "someParam": "value",
      "anotherParam": 123
    },
    "tgt": {
      "process": "Reader",
      "port": "config"
    }
  }
]

It is totally fine for a port to be used only once for configuration. It makes it explicit to the graph designer: this component requires configuration. This is exactly what IIPs exist for in FBP, and I would like to stick to the original paradigm unless implementing features that are orthogonal and not really represented in the original concept.

Re: plugins in Golang

Honestly, I didn't even know about Go plugins before this PR. And I see there are problems with them.

Question: do you need plugins to add components at run-time, or to package components separately? Is it a required thing in your use case?

If you don't need run-time evolution of the graphs, nor shipping components as binary packages, then just compiling everything as a single binary shouldn't be a big issue given Go's compiler speed.

Re: managed plugin model

Could you elaborate a bit on what you are aiming to achieve?

dahvid and others added 2 commits September 22, 2020 13:33

Added plugins feature that allows separate binaries as processes

8bac583

Used the loader which was compiled out, and had to brutilize it a bit Need a conversation about what the intention was of some of the non-working code i it

Added a PlugIn interface, that is the beginning for managed Components

d4716b5

The first layer of management is that parameters can be passed to the Plugins via the Json graph, under an optional Parameters field

trustmaster requested changes Oct 10, 2020

View reviewed changes

persist

565bf3c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added plugins feature that allows separate binaries as processes #58

Added plugins feature that allows separate binaries as processes #58

dahvid commented Sep 22, 2020

trustmaster left a comment

trustmaster Oct 10, 2020

trustmaster Oct 10, 2020

trustmaster Oct 10, 2020

trustmaster Oct 10, 2020

dahvid commented Oct 12, 2020 via email

trustmaster commented Oct 12, 2020 •

edited

Loading

Added plugins feature that allows separate binaries as processes #58

Are you sure you want to change the base?

Added plugins feature that allows separate binaries as processes #58

Conversation

dahvid commented Sep 22, 2020

trustmaster left a comment

Choose a reason for hiding this comment

trustmaster Oct 10, 2020

Choose a reason for hiding this comment

trustmaster Oct 10, 2020

Choose a reason for hiding this comment

trustmaster Oct 10, 2020

Choose a reason for hiding this comment

trustmaster Oct 10, 2020

Choose a reason for hiding this comment

dahvid commented Oct 12, 2020 via email

trustmaster commented Oct 12, 2020 • edited Loading

Re: IIPs vs. map[string]interface{} params

Re: plugins in Golang

Re: managed plugin model

trustmaster commented Oct 12, 2020 •

edited

Loading

Re: IIPs vs. `map[string]interface{}` params