Add strict validation check for core dependencies by unkcpz · Pull Request #130 · aiidalab/aiidalab-home

unkcpz · 2023-02-08T11:26:28Z

Replacement of #128

along with aiidalab/aiidalab#347 will fixes aiidalab/aiidalab#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.

after merging and relase validate the core dependencies aiidalab#347

yakutovicha

Thanks, @unkcpz, I tested the combination of aiidalab-home and aiidalab PR - they seem to work! I didn't have time yet to closely look at the code, but what I wonder is - does it make sense to show the incompatible versions? Maybe we should hide them and have a tickbox to show the incompatible ones only per user request?

Otherwise, I have no quick way to pick up a version that works for my current setup. I need to enumerate all of them to find out which version is compatible. It might be a bit tedious.

Also, I propose to change the messages a bit.

danielhollas

@unkcpz thanks so much. I don't have time to fully test this today, I'll try on Monday.
For now just a couple of minor comments on the code.

yakutovicha · 2023-02-15T12:17:51Z

After some thinking, I would propose the following:

Do not list versions that have incompatible core dependencies (a user cannot do much about them since the installation is forbidden anyways)
If no compatible version with core dependencies is found - show a banner: "No version compatible with the current environment found". (only display relevant information)
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 · 2023-02-15T12:56:43Z

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

danielhollas · 2023-02-15T14:19:38Z

@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.

We agreed to never display packages that are incompatible with the core requirements.

yakutovicha · 2023-02-17T10:46:18Z

ready for testing @unkcpz and @danielhollas

unkcpz · 2023-02-17T14:30:05Z

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.

unkcpz · 2023-02-21T17:08:08Z

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 #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 7576ec5, 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

aiidalab-home/home/app_store.py

Line 170 in 0b1c618

display(ipw.HTML("<hr>")) # horizontal line

, we can make it more obvious. But I also implement the next point, which also clarifies the case.

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

Addressed by 989262d by moving out the compatible not satisfied case out and turn off the ignore checkbox.

for more information, see https://pre-commit.ci

danielhollas

@unkcpz thanks for the changes! We're getting very close. There's a bunch of general UI improvements that could still be done but we can do later.

One last thing I noticed, when you load the single app page, the Install button flashes several times before all the versions are loaded. 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).

If this turns out to be too much more work, please just open an issue so we can go ahead with this.

unkcpz · 2023-02-23T17:40:25Z

@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 · 2023-02-23T18:00:49Z

            {% endfor %}
            </ul>
+
+            The compatibility of dependencies may break by other apps, you can uninstall and install this app again to fix it


I add additional commit with a note here to show what to do if the incompatibility happened. Usually this caused by installing another package that has different dependencies and break the compatibility of this app.

Really great idea! 👏 I suggest minor rewording. We need to make clear that only uninstalling the app does not help, because the dependencies will not be rolled back to the previous state. The user needs to:

Uninstall the app that broke the environment.

Reinstall the broken app.

Suggested change

The compatibility of dependencies may break by other apps, you can uninstall and install this app again to fix it

WARNING: Reinstalling previously installed dependencies may break already installed apps. If that happens, uninstall this app and reinstall the app that was broken.

(You may want to adda <br> linebreak to make this nicer if necessary, I haven't tried)

When an incompatibility happens, pip actually prints a warning at the end. Also pip check will detect issues. I will open a separate issue for this, we should run pip check at the end of app install and surface any problems to the user.

Oh, I misunderstood where this message is. It's in the template of the app that is incompatible. The sentence above should go to the DEPENDENCIES_INSTALL_INFO. Here I would say

Suggested change

The compatibility of dependencies may break by other apps, you can uninstall and install this app again to fix it

The compatibility issues may be caused by other installed apps. To solve this, you can re-install this app, and uninstall the app that caused the conflict.

Yes, there is. It shows when the warning box with details of which dependencies are not matched. This box only shown when the "App incompatible" is shown on the home page.
Changed.

danielhollas

During validating the version, the widget should be in the busy status. We have a _show_busy context manager to use.

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

Otherwise I have only two wording suggestions.

danielhollas · 2023-02-23T21:13:09Z

            {% endfor %}
            </ul>
+
+            The compatibility of dependencies may break by other apps, you can uninstall and install this app again to fix it


Really great idea! 👏 I suggest minor rewording. We need to make clear that only uninstalling the app does not help, because the dependencies will not be rolled back to the previous state. The user needs to:

Uninstall the app that broke the environment.

Reinstall the broken app.

Suggested change

The compatibility of dependencies may break by other apps, you can uninstall and install this app again to fix it

WARNING: Reinstalling previously installed dependencies may break already installed apps. If that happens, uninstall this app and reinstall the app that was broken.

(You may want to adda <br> linebreak to make this nicer if necessary, I haven't tried)

When an incompatibility happens, pip actually prints a warning at the end. Also pip check will detect issues. I will open a separate issue for this, we should run pip check at the end of app install and surface any problems to the user.

danielhollas · 2023-02-23T21:17:39Z

            {% endfor %}
            </ul>
+
+            The compatibility of dependencies may break by other apps, you can uninstall and install this app again to fix it


Oh, I misunderstood where this message is. It's in the template of the app that is incompatible. The sentence above should go to the DEPENDENCIES_INSTALL_INFO. Here I would say

Suggested change

The compatibility of dependencies may break by other apps, you can uninstall and install this app again to fix it

The compatibility issues may be caused by other installed apps. To solve this, you can re-install this app, and uninstall the app that caused the conflict.

unkcpz · 2023-02-23T22:25:58Z

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 aiidalab/aiidalab@39e2abc

danielhollas

Thanks @unkcpz. All good. I'll approve this once the aiidalab PR is ready and I can do the final testing.

for more information, see https://pre-commit.ci

danielhollas · 2023-03-01T17:29:19Z

@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.

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

danielhollas

Let's go! 🎉

Great work on this @unkcpz @yakutovicha, this is a great feature.

yakutovicha

I will also approve to satisfy github 😆

unkcpz · 2023-03-01T23:26:16Z

Cheers! 💪 thanks both!

unkcpz added 6 commits February 7, 2023 15:10

Strict dependencies check for aiida-core

b1148ef

pre-commit clean-up

1e04b32

f-m

8c29c10

r-d

1cc6ac1

New implementation of dep-check

6159201

f-m: layout and warning banner

b30b84c

unkcpz mentioned this pull request Feb 8, 2023

Strict dependencies check for aiida-core #128

Closed

unkcpz marked this pull request as draft February 8, 2023 11:27

unkcpz marked this pull request as ready for review February 8, 2023 15:17

Delete staled functions

153a046

unkcpz force-pushed the feat/aiidalab/318/core-dep-check branch from 6d46cce to 153a046 Compare February 8, 2023 16:36

yakutovicha self-requested a review February 9, 2023 15:27

yakutovicha requested changes Feb 9, 2023

View reviewed changes

Comment thread home/app_manager.py Outdated

Comment thread home/app_manager.py Outdated

danielhollas requested changes Feb 10, 2023

View reviewed changes

Comment thread home/app_manager.py Outdated

Comment thread home/app_manager.py Outdated

Comment thread home/app_manager.py Outdated

Comment thread home/app_manager.py

yakutovicha and others added 2 commits February 14, 2023 17:37

Rename strict_dependencies_validate with strict_dependencies_satisfied

8cc5907

Show more info for core incompatibility

7a87dca

unkcpz force-pushed the feat/aiidalab/318/core-dep-check branch from 09299e4 to 7a87dca Compare February 14, 2023 19:43

yakutovicha added 3 commits February 16, 2023 12:31

Remove core-packages incompatibility warnings:

9b94d23

We agreed to never display packages that are incompatible with the core requirements.

AppManagerWidget - add minimalistic view.

e3bbeb6

Introduce header warning (instead of compatibility warning)

ecbb8ea

yakutovicha force-pushed the feat/aiidalab/318/core-dep-check branch from 6e1cdac to ecbb8ea Compare February 16, 2023 15:28

yakutovicha added 2 commits February 17, 2023 09:45

Do not activate install button if no versions available.

0a0e177

Clean up _refresh_widget_state function

e2da69c

yakutovicha requested review from danielhollas and yakutovicha February 17, 2023 10:46

unkcpz force-pushed the feat/aiidalab/318/core-dep-check branch from a774827 to 989262d Compare February 21, 2023 17:07

unkcpz requested a review from danielhollas February 21, 2023 17:08

don't show ignore blocked checkbox if not compatible

acbca31

unkcpz force-pushed the feat/aiidalab/318/core-dep-check branch from aebe804 to acbca31 Compare February 21, 2023 17:34

[pre-commit.ci] auto fixes from pre-commit.com hooks

4502d57

for more information, see https://pre-commit.ci

danielhollas requested changes Feb 22, 2023

View reviewed changes

Comment thread home/app_manager.py

Comment thread home/app_manager.py Outdated

r-d: dup

0f18525

unkcpz requested a review from danielhollas February 23, 2023 17:40

add note for if dependencies are break by other installation.

f2abeef

unkcpz commented Feb 23, 2023

View reviewed changes

danielhollas mentioned this pull request Feb 23, 2023

Make the separator between apps in Appstore visible #142

Open

danielhollas requested changes Feb 23, 2023

View reviewed changes

r-d update warning to re-install

5007e4c

unkcpz force-pushed the feat/aiidalab/318/core-dep-check branch from d9a0f74 to 5007e4c Compare February 24, 2023 09:50

unkcpz requested a review from danielhollas February 24, 2023 10:37

danielhollas reviewed Feb 24, 2023

View reviewed changes

unkcpz and others added 3 commits March 1, 2023 17:50

Merge branch 'main' into feat/aiidalab/318/core-dep-check

90fd903

Hide ignore checkbox

c4dbdb3

[pre-commit.ci] auto fixes from pre-commit.com hooks

f306327

for more information, see https://pre-commit.ci

Merge branch 'main' into feat/aiidalab/318/core-dep-check

546a535

danielhollas self-requested a review March 1, 2023 18:42

danielhollas approved these changes Mar 1, 2023

View reviewed changes

yakutovicha approved these changes Mar 1, 2023

View reviewed changes

unkcpz merged commit 0670766 into main Mar 1, 2023

unkcpz deleted the feat/aiidalab/318/core-dep-check branch March 1, 2023 23:25

	The compatibility of dependencies may break by other apps, you can uninstall and install this app again to fix it
	WARNING: Reinstalling previously installed dependencies may break already installed apps. If that happens, uninstall this app and reinstall the app that was broken.

	The compatibility of dependencies may break by other apps, you can uninstall and install this app again to fix it
	The compatibility issues may be caused by other installed apps. To solve this, you can re-install this app, and uninstall the app that caused the conflict.

Conversation

unkcpz commented Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yakutovicha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danielhollas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yakutovicha commented Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

unkcpz commented Feb 15, 2023

Uh oh!

danielhollas commented Feb 15, 2023

Uh oh!

yakutovicha commented Feb 17, 2023

Uh oh!

unkcpz commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

unkcpz commented Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danielhollas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

unkcpz commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

unkcpz Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielhollas Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

danielhollas Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

unkcpz Feb 24, 2023

Choose a reason for hiding this comment

Uh oh!

danielhollas left a comment

Choose a reason for hiding this comment

Uh oh!

danielhollas Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

danielhollas Feb 23, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

unkcpz commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danielhollas left a comment

Choose a reason for hiding this comment

Uh oh!

danielhollas commented Mar 1, 2023

Uh oh!

danielhollas left a comment

Choose a reason for hiding this comment

Uh oh!

yakutovicha left a comment

Choose a reason for hiding this comment

Uh oh!

unkcpz commented Mar 1, 2023

Uh oh!

Reviewers

Assignees

unkcpz commented Feb 8, 2023 •

edited

Loading

yakutovicha commented Feb 15, 2023 •

edited

Loading

unkcpz commented Feb 17, 2023 •

edited

Loading

unkcpz commented Feb 21, 2023 •

edited

Loading

unkcpz commented Feb 23, 2023 •

edited

Loading

unkcpz Feb 23, 2023 •

edited

Loading

unkcpz commented Feb 23, 2023 •

edited

Loading