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

Centralized notebook for managing AiiDA codes #188

Merged
merged 14 commits into from
Jan 10, 2025

Conversation

edan-bainglass
Copy link
Member

@edan-bainglass edan-bainglass commented Dec 11, 2024

This PR introduces an external notebook to manage AiiDA codes.

image

Feel free to review, though note that this repo may not be the final placement for this PR.

from __future__ import annotations

import ipywidgets as ipw
import pandas as pd
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pandas is a very heavy dependency. Unless it's really needed we should avoid it here.

(I plan to remove pandas dependency in AWB in near future, and we don't depend on it anywhere else in the stack)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI we depend on it at least in the QE app for the job history list. Between that and this code table, we've been discussing introducing a generalized pandas table component.

That said, let me think how crucial is pandas to the widget.

Also, maybe we do want to move this PR to AWB, but then still need to think where the notebook goes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we need to think more where to put it, perhaps the most expedient way is to put it to QeApp first. I don't think it belongs to AWB, since we will soon be converting AWB into a library-only (i.e. just widgets, no apps) so it shouldn't contain any notebooks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so @superstar54 and I are discussing this. We agree with you in the sense that it would be a great component of the Control page. So, @yakutovicha, status? Could we help getting the Control page integrated, then follow up with integrating this code management there (if you don't already have something for it)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pandas dependency removed. Performance appears unaffected, though I guess this scales not as well as pandas. But I suppose that would be an issue when one's database exceeds 100 codes? Not sure how realistic this is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so @superstar54 and I are discussing this. We agree with you in the sense that it would be a great component of the Control page. So, @yakutovicha, status? Could we help getting the Control page integrated, then follow up with integrating this code management there (if you don't already have something for it)?

@edan-bainglass I didn't finish it, but I don't think there were many things to finish. I can "revive" the PR tomorrow and try to get it ready ASAP.

@edan-bainglass
Copy link
Member Author

@giovannipizzi @cpignedoli can I get your opinion on this notebook? The idea is a one-stop shop for computational resources. This improves the performance of using the code widgets in the app, as they do not need to individually probe the database, as they do now in preparing their attached code setup widget. AWB#646 allows one to opt-out of these setup widgets.

@cpignedoli
Copy link
Member

thanks a lot @edan-bainglass , I am abroad without PC , I will test this on Monday for a feedback

@giovannipizzi
Copy link
Member

Thanks, the idea looks great! Cannot test now, but from the screenshot looks very good.

Does the code get hidden as soon as you click he checkbox? If so, should those be buttons instead? How do you show hidden codes?

Indeed, this should go to a generic app to manage aiida stuff, that I think we have and we need to start rediscussing to decide what should be there, what to polish, etc (I'd put it already there, and then we discuss at the next meeting the details?)

Copy link
Member

@cpignedoli cpignedoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @edan-bainglass

  • I would check by default the box "show active codes only"
  • Check that a code is specified to avoid empty code creation:
image
  • Add information that clicking on QuickSetup: also the computer will be created if not already present, to add a code to an existing computer specifu teh correct label of teh existing computer

@edan-bainglass
Copy link
Member Author

Thanks, the idea looks great! Cannot test now, but from the screenshot looks very good.

Does the code get hidden as soon as you click he checkbox? If so, should those be buttons instead? How do you show hidden codes?

Currently, checking the box does not automatically remove the row unless you have the "show active codes only" checkbox checked, in which case it does. However, I was already thinking perhaps that checkbox should be "show hidden codes only", or maybe something else. See Carlo's comment for use cases that may need something different. Let's discuss an ideal plan for this.

Indeed, this should go to a generic app to manage aiida stuff, that I think we have and we need to start rediscussing to decide what should be there, what to polish, etc (I'd put it already there, and then we discuss at the next meeting the details?)

I think you're referring to @yakutovicha's control page (#156). In any case, I agree some default app for various general controls that are true for all apps is a good plan and place for this notebook. Let's discuss!

@edan-bainglass
Copy link
Member Author

edan-bainglass commented Dec 16, 2024

  • I would check by default the box "show active codes only"

See my last comment.

  • Check that a code is specified to avoid empty code creation:

This and other issues should maybe be handled in separate PRs, as they are already issues with the present setup widget and are not introduced in this PR. Nevertheless, equally important to handle. For example, at the moment, nothing stops you from creating the same code over and over again, which effectively breaks that code (won't be able to load it). Gotta fix this! Will open issues!

image
  • Add information that clicking on QuickSetup: also the computer will be created if not already present, to add a code to an existing computer specifu teh correct label of teh existing computer

Yes, in general, I would suggest more text in this notebook. We should maybe test it on some folk to see if the docs we end up providing are sufficiently clear. Let's discuss.

@edan-bainglass edan-bainglass force-pushed the code-setup-notebook branch 2 times, most recently from a565dae to 0605df2 Compare December 17, 2024 08:57
@edan-bainglass edan-bainglass self-assigned this Dec 17, 2024
@edan-bainglass edan-bainglass marked this pull request as draft December 17, 2024 08:57
@edan-bainglass
Copy link
Member Author

Following up regarding the placement of this notebook - I agree that it should live within some general control tool. However, this does not prevent for the time being placing the notebook in the root of this repo. It does not harm anything but does provide itself to apps already aware of it. Thoughts?

As stated by @danielhollas, it is likely best to add it to @yakutovicha's control page. And apparently there isn't much left there in order to merge it. However, we are already inundated with issues/PRs at the moment, so at least I can't allocate time at the moment to this. But I would still like access to this notebook at least from the QE app (see aiidalab/aiidalab-qe#986)

@edan-bainglass
Copy link
Member Author

Hi @edan-bainglass, I agree that the notebook can be kept in this repo for the moment and moved to another place when we figure out the place to hold it. So, please go ahead and include it in the new release.

Great! Thanks @superstar54. Could you please give a quick review? 🙏

@edan-bainglass edan-bainglass marked this pull request as ready for review December 18, 2024 13:26
)

show_all_button = ipw.Button(
description="Show All",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This button is not that clear for the user. Good to add a description in the GUI.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually plan to change it. There was some discussion with Carlo regarding what is the best option (show all, hide all, etc.). Still thinking about it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I realize now you mean the other button that unhides any hidden code. Okay, let me at least add a tooltip.

Copy link
Member

@superstar54 superstar54 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main concern is the performance. I did a quick test, for data with 90 rows, it takes ~ 2.5 s to render the table; thus, every action (search, click button) takes ~2.5 s to react, resulting in a poor user experience. While this performance may be acceptable for code setup due to the limited number of codes, it hides the usability as a general table component for other applications.

Another small issue: performing a search will override the Show active codes only

Comment on lines +129 to +131
"widget.aiida_code_setup.observe(\n",
" on_code_setup_message_change,\n",
" \"message\",\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why observe the message?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a code was created ("created" in message), I rerender the table to refresh and pick up the new code. If none create, no need to refresh.

@edan-bainglass
Copy link
Member Author

My main concern is the performance. I did a quick test, for data with 90 rows, it takes ~ 2.5 s to render the table; thus, every action (search, click button) takes ~2.5 s to react, resulting in a poor user experience. While this performance may be acceptable for code setup due to the limited number of codes, it hides the usability as a general table component for other applications.

Let's not let this future matter hold this up.

Another small issue: performing a search will override the Show active codes only

Will take a look.

@giovannipizzi
Copy link
Member

Ok for me to go forward if you guys agree, keeping an issue open to discuss improvements and different placement

@edan-bainglass
Copy link
Member Author

edan-bainglass commented Dec 20, 2024

My main concern is the performance. I did a quick test, for data with 90 rows, it takes ~ 2.5 s to render the table; thus, every action (search, click button) takes ~2.5 s to react, resulting in a poor user experience. While this performance may be acceptable for code setup due to the limited number of codes, it hides the usability as a general table component for other applications.

Let's not let this future matter hold this up.

Likely a simple way to improve performance is to shift filtering over to the database (query builder), where filtering is mostly optimized (at least w.r.t python). Other improvements can be made to the rendering itself. I'll have a look at this after the release. @superstar54 let's set up a chat regarding requirements for a generic table component.

Another small issue: performing a search will override the Show active codes only

Will take a look.

Resolved!

@edan-bainglass
Copy link
Member Author

@superstar54 can I proceed with this one?

@superstar54
Copy link
Member

Why did you change it to "Show hidden codes only"? I select it, and then click the "Show All" button, then the table became empty. This is a little strange for the user: click show all, and then the table shows nothing.
I think the previous "show activated codes only" is more suitable.

@edan-bainglass
Copy link
Member Author

Why did you change it to "Show hidden codes only"? I select it, and then click the "Show All" button, then the table became empty. This is a little strange for the user: click show all, and then the table shows nothing. I think the previous "show activated codes only" is more suitable.

"Show All" is now "Unhide All" to make it clear what it does. It deselects all checkboxes in the hide column. Please let me know if this is still not clear. So hopefully you see that "Show All" was not meant to show all rows but rather unhide all hidden codes. I plan to add documentation/instructions in various places in the notebook.

Regarding "Show hidden codes only", I think it is best to show ALL codes by default. This was the behavior before. But a user may want to quickly see all hidden codes, so they may deselect some of them. I'm open to suggestions if you think this is unclear or not useful.

@cpignedoli
Copy link
Member

I wanted to suggest using "Unhide all" so I am perfectly in line with it. I am fine with keeping show ALL as the default

@superstar54
Copy link
Member

I got one error; maybe you forgot to adjust the capitalization.

~/repos/aiidalab/aiidalab-home/home/code_setup.py in create_row(row, on_checkbox_change)
     53         layout=ipw.Layout(width="fit-content", margin="2px 2px 2px 15px"),
     54     )
---> 55     hide_checkbox.full_label = row["Full Label"]
     56     hide_checkbox.observe(on_checkbox_change, names="value")
     57     table_row = ipw.HBox(

KeyError: 'Full Label'

I suggest adding a basic test for the code to avoid such error.

The rest of the code LGTM!

@edan-bainglass
Copy link
Member Author

I got one error; maybe you forgot to adjust the capitalization.

~/repos/aiidalab/aiidalab-home/home/code_setup.py in create_row(row, on_checkbox_change)
     53         layout=ipw.Layout(width="fit-content", margin="2px 2px 2px 15px"),
     54     )
---> 55     hide_checkbox.full_label = row["Full Label"]
     56     hide_checkbox.observe(on_checkbox_change, names="value")
     57     table_row = ipw.HBox(

KeyError: 'Full Label'

I suggest adding a basic test for the code to avoid such error.

The rest of the code LGTM!

Thanks @superstar54. This is now fixed. Was referring to the old capitalization scheme, so had to update the headers.

@edan-bainglass
Copy link
Member Author

Two things left to do here:

  1. Ensure that code full labels are unique (to be addressed in the AWB widget
  2. Add instructions for users

I'll handle 1. For 2, @unkcpz I'm told you might know best these widgets. Would you be able to provide a draft set of instructions for operating the Create new code part of the notebook? 🙏 (see description). I'll add documentation for the Available codes section.

@superstar54 superstar54 self-requested a review January 10, 2025 15:06
Copy link
Member

@superstar54 superstar54 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code LGTM!

We can move the notebook to another place if we figure out a better place to hold it, e.g. the one which @yakutovicha is working on.

@edan-bainglass edan-bainglass merged commit 7b626ab into aiidalab:main Jan 10, 2025
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants