Asynchronous source code loading - Package Reloading

[HeftyCoder]

This issue discusses 2 concerns:

• Source Code Loading Delays
• "Hot" Reload for packages

For Source Code Loading

I'm documenting an idea for asynchronous source code loading, in case this might be interesting. First of all, some left-over code in gui_env.py retrieves the source code of the GUI if it exists, appends it to a list and does nothing with it. In #174 , I also added that code in the decorator function for legacy purposes, before realizing it does nothing. I believe these source code retrievals should be removed.

The source code of the node and its GUIs is captured in code_storage.register_node_type. I noticed that this function, if instance.src_code_edits_enabled is False, takes around 3-10 ms (and more) to complete, depending on the GUI code present. For example, pkg_test takes 0.7s to import, largely due to this fact. This to me is not desirable, since python scales linearly. If we have a package with 50 nodes, why should Ryven freeze for 2 or more seconds for all source codes to be loaded, when it's not really that important?

My proposed solution is asynchronous source loading. Have the source loading happen on a separate thread. If someone drags a node and that node's source loading hasn't ended, it loads the source code on the spot. I do realize this might not be worth thinking that much and requires testing, but I'm documenting it here just in case.

For "Hot" Package Reloading

Is there any reason that prevents us from having package reloading? The only thing I can think of that is hard is how to handle a flow that's already been created but the package has changed. But the same thing kind of applies when simply closing and starting Ryven again. Also, this feature might benefit from asynchronous source code loading.

This is a documentation of two ideas for better QoL in Ryven. What are your thoughts?

[leon-thomm]

Thanks for opening this.

takes around 3-10 ms (and more) to complete

Interesting, did you profile it? What part of loading is causing this, is it the actual inspect.getsource() call? Redundant sampling should be fixed but it still shouldn't be this slow. I noticed it is slow but never really looked into it.

My proposed solution is asynchronous source loading. Have the source loading happen on a separate thread.

That's the kind of complexity I want to stay very far away from. Ryven had a bunch of concurrency stuff in the past and there's good reasons it doesn't now.

Loading source code shouldn't take this long, and there's tons of things to optimize before considering concurrency.

Is there any reason that prevents us from having package reloading? The only thing I can think of that is hard is how to handle a flow that's already been created but the package has changed. But the same thing kind of applies when simply closing and starting Ryven again.

A nodes package can store any amount of static data internally, there's purposefully basically no restrictions on that. In this model, simply replacing components will get your packages into inconsistent states in no time at all.

While the concept of hot reloading doesn't seem sensible to me in Ryven's unrestricted model, a "quick reload" which simply saves the project and re-launches it with the same configuration could serve a similar purpose and very much does fit into the model. From the node developer's side this simply requires them to implement their set_data() methods in a way that a node of class SomeNode whose implementation was modified can load the state that was saved by SomeNode before, which is easy to do using the version field.

[HeftyCoder]

Interesting, did you profile it? What part of loading is causing this, is it the actual inspect.getsource() call?

I did profile it. Here's a snapshot of importing the build_in library. This takes into account only getting the source of the code and the gui (only calling inspect.getsource() sequentially), not the whole code_storage.register_node_type method. I'm mostly worried with packages with a LOT of nodes (i.e. 50 nodes could potentially take several seconds), hence the asynchronous loading idea.

--> INFO:  Registered node gui: <class 'built_in.nodes.Result_Node'> for <class 'built_in.gui.ResultGui'>
source code: 0.008931999997003004 sec
gui code: 0.0028319999983068556 sec
total: 0.01176399999530986
--> INFO:  Registered node gui: <class 'built_in.nodes.Val_Node'> for <class 'built_in.gui.ValGui'>
source code: 0.004401999991387129 sec
gui code: 0.003798300022026524 sec
total: 0.008200300013413653
--> INFO:  Registered node gui: <class 'built_in.nodes.SetVarsPassive_Node'> for <class 'built_in.gui.SetVarsGui'>
source code: 0.006081100000301376 sec
gui code: 0.004280699999071658 sec
total: 0.010361799999373034
--> INFO:  Registered node gui: <class 'built_in.nodes.SetVar_Node'> for <class 'built_in.gui.SetVarGui'>
source code: 0.006254200008697808 sec
gui code: 0.0050675999955274165 sec
total: 0.011321800004225224
--> INFO:  Registered node gui: <class 'built_in.nodes.GetVar_Node'> for <class 'built_in.gui.GetVarGui'>
source code: 0.0057560999994166195 sec
gui code: 0.003943399991840124 sec
total: 0.009699499991256744

a "quick reload" which simply saves the project and re-launches it with the same configuration could serve a similar purpose and very much does fit into the model.

I can see that, makes more sense. How would you handle potentially broken nodes? Simple try / except clause?

[leon-thomm]

Could you show a flame graph of the whole process of importing a package using a profiler?

I can see that, makes more sense. How would you handle potentially broken nodes? Simple try / except clause?

It would be nice if Ryven doesn't terminate if a package broke. Some parts like the update_event() are try-catched already, but substantial errors would just crash the loading. Since we want to re-load everything from scratch, I think we'll need to invoke a new instance of the interpreter (i.e. a spawn new process). Roughly something like

1. save project in a tmp project file
2. clone the config
3. spawn new Python process launching Ryven
4. if there's an error, show the error in the original editor and keep that running
5. if there is no error, terminate the original process and switch to the main window of the new one

Implementation wise, could look something like this (see multiprocessing)

original process

import multiprocessing as mp

def on_quick_reload(config, tmp_file_path):
    mp.set_start_method('spawn')
    q = mp.Queue()
    p = mp.Process(target=quick_launch, args=(q,config, tmp_file_path))
    p.start()
    ret = q.get()
    if ret == 'success':
        sys.exit()  # and switch to new window?
    else:
        show_error(ret)

new process

def quick_launch(q, config, tmp_file_path):
    try: 
        Ryven.run(config, tmp_file_path, on_launch=lambda _: q.put('success'))
    except Exception as e:
        q.put(e); sys.exit()  # return error to the spawning process

[HeftyCoder]

Could you show a flame graph of the whole process of importing a package using a profiler?

Currently I don't have time for this and I don't know if and when I'll be able to. If I'll ever get to it I'll do it, though I think the above results showcase the problem. I don't think asynchronously inspecting the code will be that hard to implement and it's only for a very specific part of ryven.

Implementation wise, could look something like this...

Nice, will probably check it out when I have more time.

[leon-thomm]

I added an option to defer source code loading. If used, the user needs to click "load source code" for each node type they want to inspect. Decreases import time of linalg+std by ~400ms on my computer. I think this is a suitable workaround without much increased complexity. Can I close this?

[HeftyCoder]

I added the GUI part of this in the StartUpDialog in the open PR.

I believe that the option should start as enabled. We want the users to start with the best user experience by opening the default version of Ryven. I can add this in the PR.

I could also see the code loading immediately when the user clicks on a node. If inspect does around ~5ms (from what I've so far) on average, then worst case scenario there's a node that takes 25ms to open if it has 5 source files. That's 40 fps, which shouldn't even be noticeable. I'm leaving this for you to decide, I don't have a particular preference. I can also change this if you want to.

Can I close this?

Depends. You could keep it open until editor reloading is implemented as well. Otherwise, notify me on how to proceed with the above and you can close the issue.

[leon-thomm]

closed in https://github.com/leon-thomm/Ryven/releases/tag/v3.5.0 with deferred loading