aiidalab / aiidalab-home

AiiDAlab Home App
MIT License
5 stars 9 forks source link

Add strict validation check for core dependencies #130

Closed unkcpz closed 1 year ago

unkcpz commented 1 year ago

Replacement of #128

along with https://github.com/aiidalab/aiidalab/pull/347 will fixes https://github.com/aiidalab/aiidalab/issues/318

The compatibilities only checked for the installed app. If the user going to install the app that would potentially override the aiida-core version the installation will fail in AppManager but the user is not notified before they trigger the installation procedure. In this PR, the check for the to-be-installed app is activated when the version is selected and will block the user to install the app that has incompatible "core" dependencies. The "core" dependencies now are only aiida-core. The method validate_strict_dependencies is used to check that the aiida-core version of the container is not overridden by the app.

If the "core" dependencies are not incompatible, the dependencies to be installed or updated will show in the info notes to remind the user what will be overridden.

yakutovicha commented 1 year ago

After some thinking, I would propose the following:

  1. Do not list versions that have incompatible core dependencies (a user cannot do much about them since the installation is forbidden anyways)
  2. If no compatible version with core dependencies is found - show a banner: "No version compatible with the current environment found". (only display relevant information)
  3. Only show the updates list when entering single_app.ipynb and not in the app store. (the app store output should be minimalistic as much as possible).

@unkcpz and @danielhollas, let me know if you agree with that. I will then go ahead with the implementation.

unkcpz commented 1 year ago

I fully agree with these, especially the first I proposed once. Please go ahead with it.

danielhollas commented 1 year ago

@yakutovicha I agree with what you wrote and what we just discussed. Please go ahead and once a new version is ready I'll test it. This will be a great enhancement.

yakutovicha commented 1 year ago

ready for testing @unkcpz and @danielhollas

unkcpz commented 1 year ago

thanks a lot for the updates, I did a test, and looks much better. Before reviewing in detail, I think one thing that can improve is when the http://localhost:8892/apps/apps/home/single_app.ipynb?app=quantum-espresso page loading, there flash out a red alert box and then quickly disappear. I think it comes from when the traitlets are not set the box show but then quickly the widget gets refreshed and all is fine. Do you think it is easy to fix? You can reproduce it by refreshing the singe_app.ipynb of QeApp.

Peek 2023-02-17 16-35

unkcpz commented 1 year ago

Besides the issue I mentioned above which I'll have a look today to see if it is easy to fix, I think this PR need to be merged after we merge the https://github.com/aiidalab/aiidalab/pull/347 and make a new release.

danielhollas commented 1 year ago

@unkcpz please ping me when / if you manage to solve the issue, and I will test both this and the other PRs together. Then I agree they should be merged, new version of aiidalab and aiidalab-home are released and we make a new docker stack release.

unkcpz commented 1 year ago

@danielhollas I fix the issue the alert popped in first load, which is introduced from aiidalab the validate method. Please see the fix at https://github.com/aiidalab/aiidalab/pull/347/commits/ce0c0d239ba9c5679324fa70c48d1c29cdcffd8d. It is ready to review I think.

unkcpz commented 1 year ago

IMHO the "include pre-releases" toggle should be hidden. If the user want a pre-release, they should probably go to the "manage app" page anyway. (but this is a minor point not related to this PR)

I don't fully agree with this, we have this functionality and it benefits for the user when need to keep it here. In essence, AiiDAlab encourages quick development so the pre-release should be common. I open issue https://github.com/aiidalab/aiidalab-home/issues/138 for further discussion or future implementing.

On the other hand, I don't like that the version that would be installed by clicking the install button is not shown. I'd like for it to be visible so that I know what I am installing. Something like "Latest version: XX" Above the install/uninstall buttons.

Addressed by https://github.com/aiidalab/aiidalab-home/pull/130/commits/7576ec500868d2baebd746ae636e11ef7f5c8585, it is the ready to have feature, the version to be installed is in the install button. I think it is a but that only the installed app shows the version to be installed previously.

The install / uninstall / update controls should be hidden for apps that are not compatible.

I think this is not a good idea, as a user look at the app store, I want to see that the app shown in the list have paralleled layout. If something is not shown in an app, would make me fill uncomfortable. If we look at other GUI package manager such as ubuntu software manage and apple store, the install button is always activated to allow users to install but will pop the incompatible information after the button is clicked if the version is not compatible. I think current implementation is quite similar. We cann't pop out the dialog so we deactivate the button for click.

It is a bit unclear, whether "The app is not compatible with this enviroment" belongs to the app above or below it. If the previous point is done, I'd propose to move it below the title and description instead of the install/uninstall buttons. It seems to me more logical to first read what the app is and then read the warning.

I have the same feeling about this, but instead might be more straightforward to separate by line. I try to add a border to the app manager widget. However, it looks very ugly. If you check carefully, there is already a horizontal line separate widgets. Not sure where it is implemented implemened in https://github.com/aiidalab/aiidalab-home/blob/0b1c618d72bf170b2a51192adcc6ca1727c2af02/home/app_store.py#L170, we can make it more obvious. But I also implement the next point, which also clarifies the case.

image

The footer that says "the app is not supported in the current environment" with the "Ignore" ticker should not be displayed.

Addressed by https://github.com/aiidalab/aiidalab-home/pull/130/commits/989262d5c60461069f95c388b8c7d90f0c536b3c by moving out the compatible not satisfied case out and turn off the ignore checkbox.

unkcpz commented 1 year ago

@danielhollas thanks!

I think the logic needs to be improved a bit, the button should only be enabled when all available versions are loaded. (for example, in the very beginning the button gets enabled with version "n/a"). (side note, the "refresh_state" function is crazy complicated now, would be good to add a bunch of tests and split it up in the future).

During validating the version, the widget should be in the busy status. We have a _show_busy context manager to use. You are right, the _refresh_state function is very busy and need to decouple a bit. But I don't worry that in this PR.

unkcpz commented 1 year ago

Okay, great. Shall we use it then? I don't see any such change in the last two commits.

Ah, the change is in aiidalab PR, forget to push there. I'll do it tomorrow, the change is on my office workstation 😭 UPDATE: The commit is https://github.com/aiidalab/aiidalab/pull/347/commits/39e2abc78087efb987f29eae30c4c1a5d9948d67

danielhollas commented 1 year ago

@unkcpz I noticed an extra "Ignore" checkbox in the appstore for incompatible apps. Also, this checkbox would flash at the beginning when loading the "Manage app" page. I've fixed it in the last commit, please take a look.

obrazek

(I also changed the wording of warnings a little bit)

unkcpz commented 1 year ago

Cheers! 💪 thanks both!