Package Group UI

[ghost]

This is a quick update in order to provide better package experience in the editor. Though ultimately I believe the NodesPackage system needs to be remade and possibly added to ryvencore (Changing the identifier prefix outside ryvencore - along with other things - leads to Ryven specific importing. Furthermore, classes with the same name from different packages break the importing. Couldn't importing eventually be remade to work by using python packages?)

Packages are now added in a list -per name. There's also a trick to include "subpackages". Packages can be named as {root}.{subpackage}. There are no subpackages of subpackages, it only works for one level of grouping. By clicking on the last node of a package, the corresponding nodes are shown for Ryven.

Example: Suppose we want to divide our nodes into simple and advanced.

Option 1: Simply have the folders linalg and std and they'll appear in the editor. Option 2: Bundle them into a "virtual" package, i.e test. Folders have to be renamed test.lnalg and test.std

Current display problem: Packages imported with the same prefix (i.e test.linalg and test.std) in the menu before creating the session are displaying only the prefix in the UI.

[leon-thomm]

Changing the identifier prefix outside ryvencore - along with other things - leads to Ryven specific importing. Furthermore, classes with the same name from different packages break the importing.

I don't see a problem here. Strictly speaking, ryvencore does not have an import mechanism, so there's nothing to break. Registering nodes in a ryvencore session means providing their class, and I'd like to keep it this way. ryvencore should contain only the absolute core functionality for the idea behind the project. It is not specific to Ryven - in fact ryvencore is used by projects that don't care about Ryven at all. Developer focused features like a packaging system etc. should be defined on a higher level, coupled with a particular implementation, because there's about a million different design choices.

Couldn't importing eventually be remade to work by using python packages?

Theoretically yes, but Python packaging is a pain, and conceptually Ryven node packages are not (and don't have to be) this complicated. I don't see an immediate argument against supporting it, but I think requiring it would significantly raise the entry barrier.

Packages are now added in a list -per name. There's also a trick to include "subpackages". Packages can be named as {root}.{subpackage}. There are no subpackages of subpackages, it only works for one level of grouping. By clicking on the last node of a package, the corresponding nodes are shown for Ryven.

I like this idea! Some immediate thoughts:

• I would be wary about arbitrary nesting
  • unnecessary complexity
  • basically a Python package at this point
  • possible UI issues (how do we nicely display a sub-sub-sub-sub-sub-sub-package in the editor)
• I think forcing at max 1 level of package nesting (packages and sub-packages) gives a good balance between modularity and simplicity
• Two sub-packages contained in the same package (e.g. test.std and test.linalg) will share common types (otherwise they should be separate packages), and in order to not run into import issues, the sub-packages should then rather be contained within the directory of the package (for example test/std, test/linalg).
  • One idea from the top of my head: define a function export_sub_package() in node_env; now test/std/whatever.py calls export_sub_package(), and if test/nodes.py imports test/std/whatever.py, we can easily attach test.std to test during loading of test.
  • Do you have other ideas?

[HeftyCoder]

I believe these are reasonable thoughts. Upon looking at it more carefully, there indeed seems to be no reason for more than 1 level of depth. It seems to be a really good balance for a graph-like system. I do find some utility functions in ryvencore could be useful though. It would be providing a good system for package handling from the get go.

As per the export_sub_package() idea, that's what I initially thought about implementing, but I opted for a quicker implementation, without having to intervene with the code as much (didn't want to change a lot of things). I suppose though that it should be relatively quick to implement something like this. One-two questions:

• The root package can still have its own nodes, correct?

• Wouldn't subnodes.py be a good enough name?

  EDIT: Oops, I used my other account to post here.

[leon-thomm]

I do find some utility functions in ryvencore could be useful though.

That's fair. But why not keeping Ryven-specific ryvencore utils in the Ryven repo? We could possibly detach it from Ryven a little, though, such that others can easily use ryvencore together with only Ryven's packaging. I don't think that's necessary right now but if someone ever needs this, I don't see strong reasons against it.

• The root package can still have its own nodes, correct?

I would think so.

• Wouldn't subnodes.py be a good enough name?

Hmm, maybe.

• You can't have multiple subnodes.py in the same directory, so this forces the user to define sub-directories for the sub-packages. This might be a good or a bad thing, I'm not sure
• In your example, I think I would personally prefer something like

  test
  ├── nodes.py   // defines export_nodes()
  ├── std.py     // defines sub-package "std"
  ├── linalg.py  // defines sub-package "linalg"
  └── utils.py   // not a sub-package, just utils

• In the (unexpected, but not impossible) case that we eventually do support deeper package nesting in the future, this might be a pain, you don't want to write subsubsubpackage.py

[HeftyCoder]

I can see that working, might give it a try tomorrow.

[HeftyCoder]

I noticed there is no apparent difference in the code between having subfolders or not. The real difference is that a single folder enforces different names. I believe it should be up to the user to arrange them however he likes. If both are done, we simply need to decide on naming conventions.

Firstly, I added sys.path.append(os.path.dirname(file)) in load_from_file. That way, the root nodes.py can be something like this, without having to add the path in every package.

import linalg.nodes
import std.nodes

or

import linalg
import std

depending on the way they are imported. Exporting sub-packages was as simple as:

def export_sub_package(node_types:[Type[Node]], data_types: [Type[Data]] = None, sub_pkg_name:str = None):
    """
    Exports / exposes nodes to Ryven as a subpackage for use in flows. Called from inside a subpackage
    which is imported in nodes.py
    """

    #Fetch the module name
    if (sub_pkg_name == None):
        filename = inspect.stack()[1].filename
        #TODO handling of the filename in various cases
        sub_pkg_name = filename

    #Apply the subpackage name
    NodesEnvRegistry.current_sub_package = sub_pkg_name
    export_nodes(node_types, data_types)
    NodesEnvRegistry.current_sub_package = None

All that's left is the handling of conflict when we have a subfolder with a nodes.py. But I suppose it's as easy as using the folder name when we encounter such a situation. Any more thoughts or dislikes?

[leon-thomm]

Thanks for looking into this! Some notes:

I added sys.path.append(os.path.dirname(file)) in load_from_file. That way, the root nodes.py can be something like this, without having to add the path in every package.

I think we should also remove the path again after the file is loaded. Assume the following structure:

node-packages
├── pkg1
    ├── nodes.py    // "import utils"
    └── utils.py
└── pkg2
    ├── nodes.py    // "import utils"
    ├── special.py
    └── utils.py

if pkg1 is imported first, and then pkg2, the import of utils in pkg2 might import pkg1/utils because it comes first in sys.path. The downside of removing the path is that dynamic imports at runtime (e.g. a node in pkg2/nodes.py executes import special in its update_event()) will break, but these imports are kind of avoidable so I think I would favor avoiding the first issue.

    filename = inspect.stack()[1].filename

Please don't access stack frames. This breaks invariants assumed by lots of surrounding software, such as debuggers. Been there, done that ;) This is the reason why some commands (such as import_guis()) take an argument __file__, because it's generally not possible in Python to determine who called a function without.

[leon-thomm]

I just noticed, importlib.util.spec_from_file_location() (which it uses to import a nodes package) has a submodule_search_locations parameter whose description sounds like exactly what we need, might give it a try, instead of mutating sys.path.

[HeftyCoder]

Well, I did quite a bit of digging...

Firstly, the submodule_search_locations is meant to work with exec_module and not load_module, which is deprecated. exec_module works, but you have to add the module using sys.modules[name] = mod before calling it. A full implementation looks something like this:

def get_mod_name(file:str) -> str:
        head, tail = os.path.split(file)
        _, pkg_name = os.path.split(head)
        mod_name = tail.split('.')[0]
        return f"{pkg_name}.{mod_name}" #i.e built_in.nodes

def add_mod(file:str, prefix:str = None, submodule_search_locations:list[str] = None):
    name = get_mod_name(file)
    if prefix != None:
        name = f"{prefix}.{name}"
    spec = importlib.util.spec_from_file_location(name, file, submodule_search_locations= submodule_search_locations)
    mod = importlib.util.module_from_spec(spec)
    sys.modules[name] = mod
    #print(f"{name} {sys.modules[name]}")
    return (spec, mod)

By doing that and also providing a package prefix, i.e. suppose we have pkg_test.std,.nodes you can add a module named pkg_test.std.nodes dynamically to the modules. The downside is that I have not found a way to also include pkg_test as a module using this. So, when you call import pkg_test.std.nodes in the root nodes.py, while the module does exist, the import throws an exception, because it can't find modules pkg_test and pkg_test.std, which must be set in sys.modules for importing to work. I can't find a way to find a module through only the path (essentially a namespace package). Everything I tried through importlib hasn't worked in creating a package module and then executing (importing it). If this worked, each ryven package would not collide with another.

At this point, I'm wondering if it would be just better to include the package location to sys.path and importing the whole package through there (by initialing things in init rather than nodes.py). I tried doing that but it hasn't worked, despite appending the path to sys.path and using importlib.import() or importlib.__import()__ (maybe I'm doing something wrong because I'm tired)

If you don't know how to solve any of the above, the only way I can think of it working is to rename the nodes.py files to the corresponding folder names (i.e a pkg.stk is compromised of pkg/pkg.py, pkg/stk/stk.py).

The other way is it leave it as is, appending to the sys.path and then removing it to avoid future conflicts.

[leon-thomm]

exec_module works, but you have to add the module using sys.modules[name] = mod before calling it.

Thanks for this, you're right, the previous inspect issue disappears.

I noticed I think I misread your original comment on python packages, I was thinking about packaging to PyPI. Normal Python packages (folders with __init__.py) I avoided for similar conflict reasons, but I already forgot that hadn't solved the package-relative import problem at all (e.g. std/node.py imports its sibling files literally by appending itself to sys.path first, which induces the issue as I described above). After another hour of trying trying to make it work based on module_from_spec(), I arrive at the same point.

I just wrote a quick prototype for a python packages based import mechanism with the following characteristics

• package-relative imports must go through the package name
  • e.g. std/nodes.py calls import std.special_nodes instead of import special_nodes
  • this should avoid the issue I outlined above (given that package names are unique, which they should be)
• a nodes package must have a __init__.py which should be empty by default
• ryven package initialization still happens in nodes.py

It's super simple

def load_from_file(file: str = None, components_list: [str] = None) -> tuple:
    """
    Imports specified components from a python module with given file path.
    """
    if components_list is None:
        components_list = []

    dirpath, filename = os.path.split(file)
    parent_dirpath, pkg_name = os.path.split(dirpath)
    mod_name = filename.split('.')[0]
    name = f"{pkg_name}.{mod_name}" # e.g. built_in.nodes

    if parent_dirpath not in sys.path:
        sys.path.append(parent_dirpath)
    mod = importlib.import_module(name)

    comps = tuple([getattr(mod, c) for c in components_list])
    return comps

I think I didn't consider originally that I could just add the package's parent directory to sys.path and hence have package-internal imports still be unique because the package name is then part of it. What do you think?

[leon-thomm]

If you agree this would be easiest and I don't suddenly remember other reasons why I didn't do it this way, I guess we could do that to actually support multi-module node packages, which we need for subpackages.

def export_sub_package(node_types:[Type[Node]], data_types: [Type[Data]] = None, sub_pkg_name:str = None):
    ...

To stay close to the name export_nodes(), I would rather call it export_sub_nodes() I think.

def export_sub_nodes(name: str, node_types:[Type[Node]], data_types: [Type[Data]] = None):
    """
    Exports / exposes nodes to Ryven as a subpackage of the currently loaded package for use in flows.
    Do not call this function in a node package's nodes.py file, call export_nodes instead.
    """
    NodesEnvRegistry.current_sub_package = sub_pkg_name
    export_nodes(node_types, data_types)
    NodesEnvRegistry.current_sub_package = None

[HeftyCoder]

Is there any reason for the initialization to go through the nodes.py file? Consider a pkg_test package with sub-packages std and linalg:

pkg_test
__init__
nodes.py
├── pkg1
    ├── nodes.py    // "import utils"
    └── utils.py
└── pkg2
    ├── nodes.py    // "import utils", "import special"...
    ├── special.py
    └── utils.py

Option 1) Package loading in root nodes.py:

#import root nodes here
...
#import subpackage nodes
from .std import nodes
from .linalg import nodes

Option 2) Package loading in init:

#import root nodes here
from . import nodes
#import subpackage nodes
from .std import nodes
from .linalg import nodes

This way, we simply import a python package, i.e importlib.import('pkg_test') instead of importlib.import('pkg_test.nodes). If we want the package to be able to be used outside of ryven as well though, it would be better to keep the loading in a separate .py file like now to avoid issues while importing it. The same thing applies to sub-packages (i.e nodes in a separate file, initialization in another). What do you think?

To stay close to the name export_nodes(), I would rather call it export_sub_nodes() I think.

Yes, that would be a better name.

On another note, I have 3 other separate ideas which I'm working on, in case you want to include them in this request: 1) Flow editor is the central widget, all the other widgets (Flows, Nodes, Consoles etc.) are dockable to it. Much more flexible than it currently is and should be quite fast and easy to implement. 2) Separation of GUI and logic. Any logic imports shouldn't depend on GUI code. 3) Inspector GUI. Widgets have GUIs, but I believe a dedicated window for altering a node's properties is a must for any editor.

1 should be easy to include in this pull request. 2 and 3 might take a few more days.

[leon-thomm]

Regarding your example, I think from . import nodes does not work, but the point still follows:

If we want the package to be able to be used outside of ryven as well though, it would be better to keep the loading in a separate .py file like now to avoid issues while importing it.

I think this would be more problematic than a nodes.py file requirement.

• To me it makes a lot of sense intuitively that a nodes package can be identified by a nodes.py file.
• The user might want to reserve __init__.py for their custom (Ryven independent) initialization stuff. This would be especially useful if there ever is some deferred package import mechanism at some point in the future (where __init__.py is loaded long before nodes.py). Not currently on my radar but might turn out very useful.
• I think we can certainly live with calling importlib.import_module(f"{pkg_name}.{mod_name}") instead of importlib.import_module(pkg_name) internally.

[...] in case you want to include them in this request

I don't have a strong preference on that, I think it's nice to have PRs isolated on specific features, but if you're doing all your work on one branch already and you'd have to pull that apart, I'm also fine with extending this PR.

1. Flow editor is the central widget, all the other widgets (Flows, Nodes, Consoles etc.) are dockable to it. Much more flexible than it currently is and should be quite fast and easy to implement.

Sure, if that's easy.

2. Separation of GUI and logic. Any logic imports shouldn't depend on GUI code.

That is the case to a large extent, and the main reason why ryvencore exists. ryvencore-qt defines main GUI classes for ryvencore components, ryven-editor/main GUI independent (or GUI-dependent files are only imported in a GUI environment), ryven-editor/gui assembles all Qt UI for the editor. I'm not sure further separating the UI would help maintainability. But I'm curious, what else are you thinking of separating?

3. Inspector GUI. Widgets have GUIs, but I believe a dedicated window for altering a node's properties is a must for any editor.

Do you mean something like a node creator app, or a browser-style inspector of instantiated components inside the Ryven editor? Regarding the former I'm wary about abstracting away nodes programming. I intentionally don't want to take away the coding from developing node packages, because there are lots of subtle things to be aware of when writing robust nodes packages. I'm strongly in favor of providing a higher barrier for nodes development, yielding higher quality node packages that are actually scalable. In fact Ryven did have a nodes crator application a while ago, and I decided to remove it.

[HeftyCoder]

I agree with the loading. Pushed it to work like we discussed, along with an example in the pkg_test folder. I also pushed docks for the Nodes, Flows and Console widgets. Will see what I can do about the Variables and Settings. Small preview on how it works (I made the console dock only to the bottom)

But I'm curious, what else are you thinking of separating?

I did notice that it's the case to a large extent. I also noticed though that, for the linalg test, nodes.py imports the guis and makes the connection inside the nodes themselves. Doesn't that make the nodes package ryven dependent, when we want it to be ryvencore dependent only? Perhaps I've missed something. But wouldn't defining the connection in a separate module solve the problem? (I just noticed the same problem exists by calling export_nodes inside the same module that defines the nodes, but that is trivial to solve by separating the export and the nodes to different files).

Do you mean something like a node creator app, or a browser-style inspector of instantiated components inside the Ryven editor?

I misspoke. Meant to say nodes have guis. I was in fact talking about a browser-style inspector for the instantiated nodes. Something like Unity (https://docs.unity3d.com/Manual/UsingTheInspector.html) or any other Game Engine has. I believe it is essential if a node has many configurable properties. I'm relatively new to python type-hinting, inspecting and serializing (though it mostly works the same in every oop-feature language), but I think it could be used to make a generic Node Inspector / Editor.

[leon-thomm]

Small preview on how it works

yup, just tried it myself, very cool!

I also noticed though that, for the linalg test, nodes.py imports the guis and makes the connection inside the nodes themselves. Doesn't that make the nodes package ryven dependent, when we want it to be ryvencore dependent only?

Oh no it actually doesn't. That's why I separated GUI definitions and node definitions, because I want nodes to work without GUI by default. nodes.py files call import_guis(__file__) to import GUI. In the editor, this function will just import the GUI components, but when running in headless mode, it won't.

I was in fact talking about a browser-style inspector for the instantiated nodes. Something like Unity (https://docs.unity3d.com/Manual/UsingTheInspector.html) or any other Game Engine has. I believe it is essential if a node has many configurable properties. I'm relatively new to python type-hinting, inspecting and serializing (though it mostly works the same in every oop-feature language).

Oh I think we're on the same page then. I've long wanted something like a DETAILS section (roughly where the SETTINGS is, which presents node properties and allows configuration. I would love an extension to the nodes API that allows for customized behavior by exposing properties, a bit like in Blender I think.

edit

I'm relatively new to python type-hinting, inspecting and serializing (though it mostly works the same in every oop-feature language),

The dynamics of python's type system are indeed an obstacle for any kind of API that tries to not break all the time. Type hints have no semantic meaning, I can totally do x: int = "test", but we can easily check at runtime if we can correctly parse the input and reject it otherwise. Serialization is easy as long as we don't allow users to define their own types (and corresponding parsing), but stick with python built-in types for this.

[HeftyCoder]

Alright! About the console placement, should we make it dockable everywhere? Currently it's only at the bottom (options are left, top, right, bottom). I can make it closeable, but then we have to introduce a new item in the View tab or something to re-open it.

Oh no it actually doesn't. That's why I separated GUI definitions and node definitions, because I want nodes to work without GUI by default. nodes.py files call import_guis(file) to import GUI. In the editor, this function will just import the GUI components, but when running in headless mode, it won't.

It's just that, despite it working exactly as you say, one still needs to import the node_env from ryven inside nodes.py. Most solutions I've worked with typically make the connection in a separate file or the gui file, in some form of metadata connection. At the very least, I believe that the GUI class should have which node it corresponds to, and not the other way around. If it doesn't make much sense in a python context, don't mind me :)

Regarding the DETAILS or EDITOR or INSPECTOR, I'll be making SETTINGS and VARIABLES dockable to the current flow, so it shouldn't be a problem to add as many tabs as we want. I believe due to python's dynamic structure, the best way to go is the following:

Define a GenericObjectGUI class, which takes a python object as an argument. For a generic implementation, one wants to be able to edit AT MOST all the attributes that are serialized. After deserialization, loop through the attributes, check if they are defined (keys must be defined in the serialization dict) and with a value. If they are (which they should be), make a widget based on common types, if they are a common type, or insert another GenericObjectGUI widget recursively.

This is generic enough, especially for Ryven. I'm not sure if there is a catch and this system would break. The only Ryven dependent thing though, is that nested objects must provide a way to retrieve a serialized version of themselves, perhaps a wrapper class if needed. Ryvencore doesn't provide an automatic serialization system that goes through the attributes, right? One has to explicitly define get_state() and set_state(). Which means that nested objects must also have the same functionality. Editing values through the inspector and setting them back in the class can be done automatically via callbacks I believe.

Do you find this overly complex, should we opt for something simpler?

edit

Serialization is easy as long as we don't allow users to define their own types (and corresponding parsing), but stick with python built-in types for this.

I just noticed this. Nevertheless, I think it's something that could be explored, as I defined above. If you think it can't or that I'm wrong somewhere, do tell.

[leon-thomm]

About the console placement, should we make it dockable everywhere?

sure, or is there any reason against?

I can make it closeable, but then we have to introduce a new item in the View tab or something to re-open it.

why would the way it was before (dragging the splitter handle) not work?

It's just that, despite it working exactly as you say, one still needs to import the node_env from ryven inside nodes.py.

node_env is unrelated to GUI, and will always need to be imported.

Most solutions I've worked with typically make the connection in a separate file or the gui file, in some form of metadata connection.

That's fair, but for Ryven somewhat over-complicated IMO.

At the very least, I believe that the GUI class should have which node it corresponds to, and not the other way around.

Hm I'm not sure about that. One node can have one GUI, and one GUI can be applied to multiple different nodes. But one node should never be applied to multiple different GUIs. But that is indeed only my own intuition, and I don't think there are any strong conventions about that in Python, so I'd be open to look at alternatives but I would rather postpone that for now.

Do you find this overly complex, should we opt for something simpler?

I do have some ideas. Could you open an issue?

[HeftyCoder]

edit

why would the way it was before (dragging the splitter handle) not work?

It didn't work out of the box with the dock widgets. Haven't searched if the functionality exists. They're still closable though. just not by dragging.

It seems that the log scroll view doesn't resize automatically to the log dock window. If you have any clue as to why, let me know so I can fix it quickly. Apparently, it's a problem regarding qt compilation from .ui to .py. I'm using 5.15.10. Instead of compiling to self.logger_doc.setWidget(self.logs_scrollArea), it compiles self.logger_doc.setWidget(self.loggerWidgetContents). I have no idea if that's my fault for not using qt creator correctly or the compiler's fault.

end_edit

But that is indeed only my own intuition, and I don't think there are any strong conventions about that in Python, so I'd be open to look at alternatives but I would rather postpone that for now.

Sure, no problem as of now. It doesn't really matter currently. It would probably matter if one were to use the nodes outside of Ryven.

I do have some ideas. Could you open an issue?

Sure! I'll probably be working on this a lot though (at least a prototype), since I need a quick implementation for a project I'm currently working on.

I'm leaving two examples of the various functionalities from the last push. I believe the updates are substantial and stable enough. If I'm missing something, do tell. (should I also make pyside6 the default? I believe it shouldn't be that hard) UI Quality of Life Loading a Project with saved window states

[HeftyCoder]

Also, I'd like to mention that what I find missing from Ryven the most, apart from the Inspector GUI, is the ability to run the flow only once with the click of a button and preventing any gui changes from updating the flow or executing it again. We had a discussion over mail for this a few days ago. I'll also be working on it, but I don't know how generic enough it could / should be to be included in vanilla Ryven.

The idea is configuring a graph and then running it only once or providing a custom script that determines how the graph is updated (i.e. in a for loop). This would be extremely helpful in situtations where you want to define Input Nodes that acquire streamed data, go through these specific nodes in a for loop and update them every N Frames / second. Or simply for running your flow only once and outputting the results.

[leon-thomm]

Apparently, it's a problem regarding qt compilation from .ui to .py. I'm using 5.15.10. Instead of compiling to self.logger_doc.setWidget(self.logs_scrollArea), it compiles self.logger_doc.setWidget(self.loggerWidgetContents). I have no idea if that's my fault for not using qt creator correctly or the compiler's fault.

I actually have no clue, if the problem is reducible to a simple example someone in the qt forum can maybe resolve it in a matter of seconds.

I'll probably be working on this a lot though (at least a prototype), since I need a quick implementation for a project I'm currently working on.

Just make sure you do that on a different branch and not clutter this PR too much, otherwise it will take long until I can merge.

I'm leaving two examples of the various functionalities from the last push.

What you show looks cool! If I notice sth once I try, I will tell.

should I also make pyside6 the default?

pyside6 is not the default because currently it breaks a few things in buggy ways, but I think I didn't take the time yet to track everything down. If you're working on pyside6 and come across particular bugs, fixes would be highly appreciated, but preferable in another PR.

[leon-thomm]

Also, I'd like to mention that what I find missing from Ryven the most, apart from the Inspector GUI, is the ability to run the flow only once with the click of a button and preventing any gui changes from updating the flow or executing it again.

As I noted in my email response, I don't think this kind of flow execution control should be part of Ryven right now, since it's highly use-case specific and can be implemented within node packages. For example, instead of using connections to distribute actual streams of data, it can make sense to instead use connections to distribute handles to the data streams which are controlled externally (like a socket, or a file descriptor).

If strong patterns and repeating use cases emerge in the future, I'm totally open to look at possible integrations, but for now I'd stick with node packages (and rather extend node package capabilities - as we're doing right now - instead of the editor).

[leon-thomm]

Is there any urgency on your side to have changes committed here be merged into master soon?

[HeftyCoder]

Just make sure you do that on a different branch and not clutter this PR too much, otherwise it will take long until I can merge.

Already doing that!

If strong patterns and repeating use cases emerge in the future, I'm totally open to look at possible integrations, but for now I'd stick with node packages (and rather extend node package capabilities - as we're doing right now - instead of the editor).

I believe the case for a one-time play button mode is a strong one. A flow can include big data processing, so it wouldn't be meaningful to have changes induce a new evaluation; it would slow down the editor too much. You could pretty much emulate this I guess by packages and custom guis, but I think it's too generic a case. Anyways, I'll be doing that on another branch, so no worries, just thought I'd mention it.

Is there any urgency on your side to have changes committed here be merged into master soon?

Not really, but I'll be basing other branches on this one, so I don't know if any conflicts will arise later on (just trying to be cautious). Time I'll be attempting to utilize Ryven is roughly 2 months; After that it's indefinite on how much I'll be able to contribute.

I've pretty much finished what I wanted to do in this PR with the last few pushes. When you have some time go through them and ask me anything if you need clarification (I may have some typos in there). Some questions for a last push:

1) Should set_performance_mode() in Design.py emit the signal even if the same performance mode is being set? Currently, it's that way. I could change it to guard against the same value if you want to. 2) Which is the correct setting to connect dock animations (currently they are always animated). Is it performance mode or animations?

[leon-thomm]

Time I'll be attempting to utilize Ryven is roughly 2 months; After that it's indefinite on how much I'll be able to contribute.

cool

1. Should set_performance_mode() in Design.py emit the signal even if the same performance mode is being set?

Sure, you can change that if I didn't comment that it's required in the code.

2. Which is the correct setting to connect dock animations (currently they are always animated). Is it performance mode or animations?

performance mode

[HeftyCoder]

As I was testing, I noticed that, despite having successfully implemented one layer sub-packaging, the import_guis function leads to some problems. Consider this structure:

pkg_test
├──__init__
├──nodes.py
├── linalg
    ├── nodes.py 
    └── gui.py
linalg
    ├── nodes.py 
    └── gui.py

Due to the nature of import_guis, which relies on the new load_from_file, the file which will be imported is the one that resides on the root linalg folder and not the subpackage one. Personally, import_guis was quite unintuitive to me and this problem has made me consider another solution. As it currently stands, my solution is this:

1) import_guis and export_guis are no longer needed. We define a node_widget or node_gui decorator:

def node_widget(node_cls:type[Node]):
    """
    Registers a node widget as the GUI for a specific node. Works with inheritance
    """
    if not issubclass(node_cls, Node):
        raise Exception(f"{node_cls} is not of type {Node}")

    def register_gui(gui_cls:type[NodeGUI]):
        print(f"Registering node gui: {gui_cls} for {node_cls}")
        node_cls.GUI = gui_cls
        GuiClassesRegistry.exported_guis.append(gui_cls)
        GuiClassesRegistry.exported_guis_sources.append(inspect.getsource(gui_cls))
        return gui_cls
    return register_gui

2) The connection happens where we define the GUI classes. As an example, I'll use the linalg packages classes. This is the gui.py

from .nodes import *

@node_widget(MatrixNodeBase)
class MatrixNodeBaseGui(NodeGUI):
   ...

@node_widget(EditMatrixNode)
class EditMatrixNodeGui(MatrixNodeBaseGui):
   ...

3) Finally, importing happens in the root nodes package. This is nodes.py directly under the folder pkg_test.

import os

from .std import nodes
from .linalg import nodes

if os.environ.get('RYVEN_MODE') == 'gui':
    from .std import gui
    from .linalg import gui

I believe this is much better than what we currently have. It removes having to call a method to import the guis and allows for plain old python importing to make the connections. For a final touch, I would probably have the definition of the nodes and the exporting happen in different files as well. That way, the definition file is completely ryven independent and can be used outside of it without having to worry about import errors.

I have this already working. What do you think?

[leon-thomm]

Thanks for the example, that's true. I like the decorator approach visually, it seems more pythonic.

Side-note: node_widget should be called node_gui IMO, it's not a widget (as opposed to e.g. main widget).

import os

from .std import nodes
from .linalg import nodes

if os.environ.get('RYVEN_MODE') == 'gui':
    from .std import gui
    from .linalg import gui

It removes having to call a method to import the guis [...]

...and replaces this requirement by boilerplate code that depends on ryven environment variables?

Concerns I have:

• runtime dependency on environment variables
  • ...defined by another (part of the) application is generally a bad idea down the line
  • runtime behavior should not depend on environment variables, but on the API (node_env and gui_env)
  • an easy solution might be to define another decorator gui_init() and decorate a function in nodes.py which imports the gui files
• complexity
  • doing the import centrally somewhere else (nodes.py) potentially decreases modularity
  • as you can see, I avoided this by doing the import in node files
• I still have the concern from above, which I don't see addressed here
  • I can't think of solutions to that other than checking at runtime that the node package behaves as expected, which might difficult to do
  • also, how do you override gui classes for node subclasses? doesn't that depend on the order in which you declare (i.e. decorate) your gui classes?
    • e.g. NodeBaseGui is a gui for NodeBase but we want to override CustomNodeGui(NodeBaseGui) for CustomNode(NodeBase)

[HeftyCoder]

I've already called it node_gui in my implementation. I can't quite see these concerns as valid. Consider the following structure:

built_in
├── nodes.py
├── nodes_def.py 
└── gui.py

1) nodes_def.py defines the nodes, as well as a list of the potentially useful nodes for exporting, called i.e nodes_to_export. It only depends on ryvencore and not anything else. 2) gui.py depends on the nodes_def and ryven to assign the gui to different nodes. Essentially, the node_gui decorator imports the node to ryven. At this point, gui_env isn't really needed. From what I could see, it just stored the guis in a list so that last one could be used and also defined a function to return a container for the gui importing inside the file where the nodes were defined. 3) The nodes.py file handles all the importing. Essentially, it is the file where users define their package. As for the runtime dependency on env variables, should be easily solvable. E.g:

from ryven import in_gui_mode #e.g for environment checking
from ryven.node_env import export_nodes
from .nodes_def import nodes_to_export

export_nodes(nodes_to_export)
if in_gui_mode():
    from .gui import *

from .linalg import nodes # subpackage 
....

I believe this is more clear and concise. Logic (i.e nodes) aren't dependent on ryven and don't import guis, Guis register themselves to nodes and depend on the nodes and ryven; And a single file defines the package. Granted, no one prohibits that you do the export and the node definitions in one file, it's just more coupled.

EDIT--------- The other way I can think of this happening is by doing this automatically when importing a package. For example, if ryven is in gui mode, check the package directory for all gui.py files and import them. That constraints the name of the file to gui, which is not bad. But one could argue that the same thing can be done with the node files as well, making the process completely automatic. I would argue that currently having the package developer define the package is more modular. Perhaps automatic importing can be discussed more in the future. END---------

also, how do you override gui classes for node subclasses? doesn't that depend on the order in which you declare (i.e. decorate) your gui classes?

Re-check the node_gui / node_widget decorator. It assigns a GUI class attribute to the node class which refers to the gui class. It's exactly the same thing we previously had via import_guis but transferred to the gui file and "type-checked". Reposting my example with the linalg for clarity:

from .nodes import *

@node_gui(MatrixNodeBase)
class MatrixNodeBaseGui(NodeGUI):
   ...

@node_gui(EditMatrixNode)
class EditMatrixNodeGui(MatrixNodeBaseGui):
   ...

EditMatrixNode is a subclass of MatrixNodeBase. The @node_gui(EditMatrixNode) overrides the GUI for the EditMatrixNode. If I remove that line, then the EditMatrixNode will use the MatrixNodeBaseGui its MatrixNodeBase base class uses.

This is tested and working (with the pkg_test package, which includes linalg and std as subpackages - all refactored to work like this), I'm currently avoiding a push as to not clutter this PR and to establish that this is something you agree with.

[leon-thomm]

I've already called it node_gui in my implementation.

good

At this point, gui_env isn't really needed.

I'd like to keep it, to potentially extend GUI API in the future.

from .nodes_def import nodes_to_export

For simple examples, I would stick with node definitions in nodes.py (questionable name otherwise), you are of course free to do that differently in your packages.

if in_gui_mode():
    from .gui import *

Let's make it a function so that we could defer GUI imports.

The other way I can think of this happening is by doing this automatically when importing a package.

I'm not opposed to that, but if explicit imports are possible without boilerplate, explicit import sounds like less problems.

Re-check the node_gui / node_widget decorator.

I explained that poorly, I was thinking towards multi-inheritance, but I this the same issues arise in the current system.

I still have the concern from above, which I don't see addressed here

...still have that, but after consulting other popular uses of decorators it seems common practice to expect unique usage, so it's fine for me.

[HeftyCoder]

Let's make it a function so that we could defer GUI imports.

Could you give an example?

...still have that, but after consulting other popular uses of decorators it seems common practice to expect unique usage, so it's fine for me.

I'm missing something. The node_gui decorator can be stacked multiple times. We could even make it be node_gui(*args) if we want to connect more nodes with the same gui in the same decorator call. I can see the problem with multiple inheritance, but then the developer just has to explicitly define which gui is to be used for that node.

Regarding this feature, would you like me to push it in this PR?

[leon-thomm]

Could you give an example?

def load_ui():
    from .std import gui
    from .linalg import gui

or

@on_load_ui()
def whatever():
    from .std import gui
    from .linalg import gui

I can see the problem with multiple inheritance, but then the developer just has to explicitly define which gui is to be used for that node.

@node_gui(Node1)
class MyGui...

# (lots of code)

@node_gui(Node1)
class SomeOtherGui...

or much worse even

@node_gui(MutableMatrixNode)
class SomeNodeGui...

@node_gui(DataFrameExtensionsBase)
class SomeOtherGui...

class MutableDFMatrix(MutableMatrixNode, DataFrameExtensionsBase)...

These mistakes are not fully obvious to me. As I said, the same is possible currently, it just seems slightly less likely.

Regarding this feature, would you like me to push it in this PR?

I would prefer another PR, if that doesn't complicate things significantly for you.

[HeftyCoder]

I don't quite understand the examples. What benefit would the developer of a package have by wrapping the imports of the UI in a function? The function must still be called. I can see the decorator doing the work of the if in my example, but someone still has to call that function in nodes.py. Is there a more important reason I'm missing?

As for the decoration of the GUI, I think at that point it's a developer's mistake. All we can do is keep a set of all the nodes types whose guis have been explicitly set and throw an exception for safety purposes. As you said, though, I don't think it's that big a problem.

I'll be waiting for this PR to close before I start a new one.

[leon-thomm]

I don't quite understand the examples. What benefit would the developer of a package have by wrapping the imports of the UI in a function? The function must still be called. I can see the decorator doing the work of the if in my example, but someone still has to call that function in nodes.py. Is there a more important reason I'm missing?

Ryven would call it. Currently this would happen at package import time but it doesn't have to, deferring GUI imports has already been on my radar. It would open some use cases and push the developer to avoid GUI-dependent semantics.

I think I prefer the decorator option, it doesn't really matter for the package dev but gives Ryven more control.

As for the decoration of the GUI, I think at that point it's a developer's mistake. All we can do is keep a set of all the nodes types whose guis have been explicitly set and throw an exception for safety purposes. As you said, though, I don't think it's that big a problem.

yeah, we might just implement some sanity checks here, but it's not top priority

I'll be waiting for this PR to close before I start a new one.

I'll try to look over all the code asap as soon as the packaging changes are done.

[HeftyCoder]

Ryven would call it

Got it. Should look nicer.

I'll try to look over all the code asap as soon as the packaging changes are done.

The last push in this PR had all the functionality I wanted to add with the packages and the UI quality of life. If I've forgotten anything, do tell. Otherwise, should be ready for reviewing.

[leon-thomm]

notes on semantics:

• fix auto discover (I can do it after merge) [^1]
• be careful with Python features of recent versions, Ryven should run on 3.6+
  • T1 | T2 union type notation was introduced in Python 3.10
  • please re-check the rest of your changes, in case I missed sth
• whenever you add a new feature, you should add basic backwards compatibility
  • your window geometry loading crashes on old projects which don't have the entry

notes on styling:

• please try to stay closer to the rest of the code base
• a PEP 8 editor plugin might help you
• includes a space between a colon and type hints, a space in a comment after "#", docstrings (instead of comments) to annotate functions, starting multi-line dicts with a newline (and then a single indentation), and consistent number of newlines between methods

I can fix the styling on your branch if you prefer.

[^1]: package auto discover is broken with sub-packages, should replace

        for top, dirs, files in os.walk(packages_dir):
            path = pathlib.Path(top)

in `StartupDialog.py` with
```python
    for entry in filter(lambda e: e.is_dir(), os.scandir(packages_dir)):
        path = pathlib.Path(entry.path)
```

[HeftyCoder]

Do you find using the black formatter disruptive? It's PEP 8 compliant. The only thing that one might dislike is that its default settings force the use of double quotes for strings, which I can disable. (meaning that formatting doesn't consider quotes, it can be either single or double)

If not, I'm using VS Code to develop. If you have a specific formatting plugin you're using, tell me and I'll use that.

[leon-thomm]

A formatter is just as good as its configuration and I don't have a formal configuration of Ryven code base styling. There are many choices even beyond PEP 8 that require additional circumspection. Of course you are free (and encouraged) to use it, but you should still make sure the styling matches the rest of the code base.

[HeftyCoder]

fix auto discover (I can do it after merge)

Fixed

Ryven should run on 3.6+

Removed any new union type notation. Furthermore, I had a lot of generic standard type hinting, i.e dict[str, str] which, as I understand, is recommended from 3.9+, as the typing module has been deprecated for standard collections. Fixed them.

your window geometry loading crashes on old projects which don't have the entry

fixed, tested by loading the example projects

please try to stay closer to the rest of the code base

Formatted using the aforementioned formatter. Styling obeys the rules you mentioned. The way some imports are typed have been affected as well [from . import N1, N2, .... Nn] are broken in lines if there are too many imports happening

As per the 3.9+ type hinting, I find it quite intuitive and a must for modern python. I commented with a should be... in all the files what the type hint would be if it were 3.9+ (I guess the typing module could be used instead). Is there any particular reason the min requirement is 3.6 or do you have any plans to update the min requirement to 3.9? (3.8 will be reach end-of-life in 2024)

[HeftyCoder]

I've checked my last commits for Ryven. Tried it for 3.6, 3.7, 3.8 (3.9+ is working as expected).

• For python 3.6, 3.7 it crashes with a syntax error: SynatxError: annotated name 'instance' can't be global in line 69 of config.py. Haven't attempted to resolve it.
• For python 3.8, it crashes with a type error in std_input_widgets.py because type[Data] is used, which is allowed in 3.9+

These aren't changes I made. I've tested this PR in 3.8 after fixing the issue and it works fine. Haven't tested in 3.6, 3.7. As it currently stands, the main branch for Ryven works with 3.9 and above.

I have finished with my changes. Please look them over. I'm leaving the 3.6, 3.7 and 3.8 issues to you (although I genuinely believe bumping the min req to 3.7 or 3.8 or even 3.9 wouldn't be bad, considering 3.8 will reach end of life next year)

[leon-thomm]

I might do minor cosmetic changes to the restyling but not in this PR

[leon-thomm]

• For python 3.6, 3.7 it crashes with a syntax error: SynatxError: annotated name 'instance' can't be global in line 69 of config.py. Haven't attempted to resolve it.

thanks that's good to know

do you have any plans to update the min requirement to 3.9?

I don't have plans to move further into the present than what's currently alive (e.g. 3.8). Now 3.6 has reached EOL long ago (3.7 also) but I still see people using it sometimes, so if it's not a major difference for us I guess we can keep it flexible.

[HeftyCoder]

Good to know!

I might do minor cosmetic changes to the restyling but not in this PR

Yeah, I'm also not a fan of some of the things it did.

Regarding af91367, there's another type hint I forgot right under the one you fixed. I can fix it in the PR for the gui decorators if you want to.

I still see people using it sometimes, so if it's not a major difference for us I guess we can keep it flexible

I was mainly talking about the library I mention in #173 which has a 3.7 min req, but other than that, I don't think it's a problem. Regarding that issue, I believe we should avoid ryven specific stuff and simply require a function or a class be implemented that attaches the Inspector it creates to a ryven window. I'll comment on that issue again and I'll wait for your reply there, I'm currently testing this.

As for the GUI decorators and deferring of gui loading, it's ready. I'll be making a new PR unless there is something you want to clarify first.

[leon-thomm]

there's another type hint I forgot right under the one you fixed. I can fix it in the PR for the gui decorators if you want to.

oh how did I not see that; I'll fix it when I look into the other version compatibility issues

I was mainly talking about the library I mention in #173 which has a 3.7 min req, but other than that, I don't think it's a problem. Regarding that issue, I believe we should avoid ryven specific stuff and simply require a function or a class be implemented that attaches the Inspector it creates to a ryven window. I'll comment on that issue again and I'll wait for your reply there, I'm currently testing this.

The concept of a mutable tree of properties could be very useful beyond the UI. To make future extension easy, this system should be really simple, and optimally prevents misuse by design. Looking forward to see what you're thinking of.

As for the GUI decorators and deferring of gui loading, it's ready. I'll be making a new PR unless there is something you want to clarify first.

Cool! Nothing from my side.

[HeftyCoder]

Sorry for posting in a closed PR, but I just realized... Lets say we have this structure.

pkg # root pkg
├── nodes.py # root nodes for pkg
├── sub_nodes.py # a sub nodes grouping of pkg
├── sub_nodes_2.py # another sub nodes grouping
└── sub_pkg  # a sub pkg 
    ├── nodes.py  # root nodes for sub_pkg
    └── sub_nodes.py  #  sub nodes grouping of sub_pkg

Don't we already have a completely defined tree - as nested as we like - for any package? If cls is the class name, then by using {cls.__module__}.{cls.__name__}, we automatically create the following:

pkg.nodes.<class>
pkg.sub_nodes.<class>
pkg.sub_nodes_2.<class>
pkg.sub_pkg.nodes.<class>
pkg.sub_pkg.sub_nodes.<class>

With this, we can easily define a tree:

pkg 
├── Node1 # from pkg.nodes
 ...
├── NodeN
└──sub nodes
   ├── Node1 # from pkg.sub_nodes
    ...
   └── NodeN
└──sub nodes 2
   ├── Node1 # from pkg.sub_nodes_2
    ...
   └── NodeN
└── sub pkg 
   ├── Node1 # from pkg.sub_pkg.nodes
    ...
   ├── NodeN
   └──  sub nodes
        ├── Node1 # from pkg.sub_pkg.sub_nodes
         ...
        └── NodeN

In this context, the string nodes. (and hence the nodes.py files) is special. Do you find any fault in this?

[leon-thomm]

What's the issue here? Of course you can have arbitrary sub-python-package nesting, but not sub-ryven-package nesting.[^1]

In this context, the string nodes. (and hence the nodes.py files) is special.

not sure what you mean by this

[^1]: we currently don't enforce this, though. it's on my TODO to enforce that a Ryven package calls export_nodes() exactly once, while export_sub_nodes() can be called arbitrarily often

[leon-thomm]

also, I'm wondering now, in order to increase flexibility and decrease confusion potential between python and ryven packaging, maybe should relax export_sub_nodes() to receive the name directly and allow it to be called anywhere.

e.g.: nodes.py

from ryven.node_env import ...

# some basic node defs
export_nodes(...)

# some node defs
export_sub_nodes('special_nodes', ...)

# some more node defs
export_sub_nodes('more_special_nodes', ...)

what do you think?

[HeftyCoder]

What's the issue here? Of course you can have arbitrary sub-python-package nesting, but not sub-ryven-package nesting.

The proposed method above does the following:

• Removes the need of the export_sub_nodes function.
• Provides multi-nesting out of the box by aligning ryven's package system with python's package system. The two are kind of similar, so merging them could make sense.
• Defines a clear and concise way to define packages. This can be good and bad, depending on how you see it.

not sure what you mean by this

Using the method above, a python package class that is pkg.sub_pkg.nodes.<class> will result in ryven package that is pkg.sub_pkg.<class>. Hence, any .nodes generated in the path by the python module should be excluded in ryven.

Essentially it creates a ryven package tree using python package rules and the special nodes.py file.

also, I'm wondering now, in order to increase flexibility and decrease confusion potential between python and ryven packaging, maybe should relax export_sub_nodes() to receive the name directly and allow it to be called anywhere.

The export_sub_nodes() function already provides an optional parameter for the sub-package name. I proposed this method to conveniently merge python and ryven packages and provide arbitrary nesting, since python already provides a tree-like structure for its packages.

I believe it solves a lot of problems. If you think the tight coupling with the python package system or the special rule of the nodes.py file do more bad than good, then it's ok. Otherwise, we pretty much have complete nested packaging without the need of an export_sub_nodes() or any other system, as showcased in the example above.

[leon-thomm]

Certainly one can also just export a sub-package with name pkg.sub.sub.subpkg which makes no sense so we should probably prevent it. We can easily implement arbitrary ryven package nesting, but the point is we we didn't want to, right? Are you re-considering that or am I misunderstanding?

As I understand it you propose to change export_nodes() to have exactly the interface that export_sub_nodes() currently has. Then, export_nodes() can be called in any nodes.py file, also in sub-(python-)packages which will yield a tree of Ryven node sub-packages which can be arbitrarily deep.

Is that correct? In that case export_nodes() will require __file__, right?

[HeftyCoder]

Are you re-considering that or am I misunderstanding?

I indeed am reconsidering this. I also believe it won't have a problem with UI issues. Let the developers define their packages as well as they like.

Is that correct? In that case export_nodes() will require file, right?

export_nodes can be called from anywhere the way I see it. But no, the __file__ is no longer required. By exporting a node you have access to the full python generated module path. In simple python code, if you have a node_type, then

s = f'{node_type.__module__}.{node_type.__name__}' # same thing can be applied to data types.

That way, a class FooA residing in a nodes.py file of package pkg will have a path of pkg.nodes.FooA. While another class FooB residing in sub_group.py file of the same package will have a path of pkg.sub_group.FooB. Repeat that for any exported nodes defined in any sub-folders and you get what I have here.

Or almost the same thing. Every class' path residing in a package's or sub-(sub-...)-package's root is "polluted" with the .nodes substring, which is the result of the nodes.py file which is used in the importing process. That's why .nodes is a special substring. Removing it gets you to:

• pkg.FooA // a class in the root pkg
• pkg.sub_group.FooB // a class in the sub_group of pkg, sub_group comes from a .py file
• pkg.sub_pkg.FooC // a class in the sub_pkg of pkg, sub_pkg comes from a folder (pkg.sub_pkg.nodes.FooC)
• pkg.sub_pkg.sub_group.FooD // class in sub_group of sub_pkg of pkg
• etc...

or in a tree structure:

pkg 
├── FooA 
└──sub_group
  └── FooB
└──sub_pkg
  ├── FooC
  └──  sub_group
    └── FooD

We're essentially letting python create the package tree by deciding on a simple rule for any nodes. we find in the module path. Only export_nodes is needed and can be called from anywhere. The fun part is that this method is less code than what we currently have. The downside to this is relying on python's module system wouldn't allow you to bundle nodes from different files the way you did in std, but it's easily fixable with a parameter in export_nodes that enforces a specific full path for that list of nodes. Note that the identifier can be the full module path. We're only removing the nodes. substring to create the tree structure.

As a side-node, so I don't forget, I believe the general approach to exporting in any file should be:

try:
    from ryven.node_env import export_nodes
    export_nodes(...)
except:
    pass

which makes the package completely ryvencore compatible.

(Also, I can even argue that export_nodes isn't really needed, but I think doing anything like that needs inspecting and will slow things down)