Added plugins feature that allows separate binaries as processes

dahvid commented 3 years ago

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.

dahvid commented 3 years ago

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.

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.
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 notifications@github.com wrote:

@trustmaster 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:

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.

In graph.go https://github.com/trustmaster/goflow/pull/58#discussion_r502784493:

@@ -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 https://github.com/trustmaster/goflow/pull/58#discussion_r502784566:
// 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 https://github.com/trustmaster/goflow/pull/58#discussion_r502785537:

@@ -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 https://github.com/trustmaster/goflow/pull/58#discussion_r502785744:

@@ -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 https://github.com/trustmaster/goflow/pull/58#pullrequestreview-506093206, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABWSOM7MG6Y62MH65W3M2FDSKBIULANCNFSM4RVV5Y3Q .

trustmaster commented 3 years ago

@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?

trustmaster / goflow