Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance plus gui input issue #150

Closed
JNmpi opened this issue Dec 20, 2022 · 4 comments
Closed

Performance plus gui input issue #150

JNmpi opened this issue Dec 20, 2022 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@JNmpi
Copy link
Contributor

JNmpi commented Dec 20, 2022

Thanks @liamhuber for the many improvements. It is particularly nice to have now nodes for the different calculators available. A few thoughts/issues below:

  • Responsiveness:
    - Adding and working with nodes having many input ports feels (as you mentioned already) very slow. This makes it very hard and sometimes impossible to work, since one is never sure whether the doubleclick etc. worked or not.
    - I had also the impression that commands like remove job takes much longer than when running pyiron directly
    - Solving and improving this issue has for me highest priority. The present waiting times are too long for productive applications or to play around and test ironflow.
    - A few ideas:
    - I fully agree with using the features provided by ryven as suggest by you in the respective issue.
    - It may be generally a good idea to show not all possible inputs in the graphic node representation but only the mandatory ones. The less important ones could be then set in the node input gui. If one needs a port to connect them to other nodes one should have an option to make them visible in the node.

  • GUI input issues:
    - When working with the calc_md node I could not set the initial_temperature variable to None. I guess since the gui ipywidget element is an integer input it accepts only integer numbers, not None. The same applies e.g. for the pressure input. For these cases we may combine an integer input gui with a boolean flag (for None or integer).

@JNmpi JNmpi added the bug Something isn't working label Dec 21, 2022
@liamhuber
Copy link
Member

Yes, performance is also a pretty high priority for me right now, although I don't expect to get to it in December due to the holidays.

When working with the calc_md node I could not set the initial_temperature variable to None. I guess since the gui ipywidget element is an integer input it accepts only integer numbers, not None. The same applies e.g. for the pressure input. For these cases we may combine an integer input gui with a boolean flag (for None or integer).

It absolutely should be a float widget not int, that was just an oversight on my part. The problem is that I have not found a way for the ipywidgets to natively support X | None types. However, there is a solution for this one! If the value defaults to None (as both your examples do), you can disconnect the port from any input and hit the "reset" button in the node controller panel to get back to the default value, even if it's incompatible with the widget. This isn't documented anywhere yet, but I'll write about it somewhere easy to find prior to the 0.0.2 release.

@liamhuber
Copy link
Member

I'm sure there is inefficiency in my canvas redraws, which could be improved by better use of events, but I suspect the biggest problem lies with ipywidgets (and my use of it).

Apparently without explicitly calling close() on each widget, references to it stay stored even after the objects get deleted. There is also one linked issue on that thread about someone running into all references still not being purged after closing, a good hint about Output also caching stuff, and a link to a library that wraps ipywidgets in what it claims is a life-cycle-safe way.

@liamhuber
Copy link
Member

Yeah, indeed, just monitoring len(ipywidgets.Widgets.widgets) shows constant growth as the node controller is opened and closed. After playing around with some smaller nodes, selecting a bigger node (MatPlot) almost doubled the count form 3xx to 5xx, which is also consistent with the perceived performance hit with the "bigger" nodes.

The good news is, this gives an extremely concrete place to start working on the performance issues.

@liamhuber liamhuber mentioned this issue Jan 11, 2023
6 tasks
@liamhuber
Copy link
Member

There is still room for improvement, especially when opening the node controller for the bigger nodes, but as of #152 it's gone from "completely unusable" to "alright". Critically, I plugged a memory leak where working with the graph created more and more ipywidgets widgets in the background. The slowness that's left is at least constant in nature, and the example notebook is totally usable again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants