Closed HeftyCoder closed 3 months ago
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.
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?
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
tmp
project fileconfig
spawn
new Python process launching RyvenImplementation 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
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.
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?
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.
closed in https://github.com/leon-thomm/Ryven/releases/tag/v3.5.0 with deferred loading
This issue discusses 2 concerns:
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, ifinstance.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?