Bump aiida-core to v2.4.0 and aiidalab to v23.3.2 #396

unkcpz · 2023-07-10T13:23:11Z

~~The workflow triggering paths are added and~~ bump the version of aiida-core to v2.4.0

danielhollas · 2023-07-10T13:54:55Z

AiiDA 2.4.0 is very recent so based on past experience maybe I would wait a bit before we switch (unless there is something that you need ASAP?). Also there's a database migration, with some potential manual steps, so we'll have to test carefully.

unkcpz · 2023-07-10T14:07:02Z

Fair, I separate the changes to #397

unkcpz · 2023-10-16T12:24:08Z

We are about to release aiida-core v2.5.0 soon. So this one we can start to try.
I also think maybe we can always follow the aiida-core minor version here in main branch. Because merge to main will influence the edge tag rather and only when making new release we decide whether to cherry-pick the version bump of aiida-core, what do you think? @danielhollas

danielhollas · 2023-10-16T12:40:01Z

We still need to figure out / test how the DB migration is handled.

unkcpz · 2023-10-16T12:52:52Z

We still need to figure out / test how the DB migration is handled.

As I know, it handled by these command

aiidalab-docker-stack/stack/base/before-notebook.d/40_prepare-aiida.sh

Lines 73 to 74 in 2d35cca

    
           # Migration will run for the default profile. 
        
           verdi storage migrate --force

But it runs automatically without notifying the users, which might cause some issues, I think @yakutovicha had more expedience on this topic.

yakutovicha · 2023-10-17T08:45:48Z

But it runs automatically without notifying the users, which might cause some issues, I think @yakutovicha had more expedience on this topic.

This is precisely the migration line. What exactly is the issue?

unkcpz · 2023-10-17T09:01:27Z

I can not foresee any immediate issues, when I wrote this I was think about maybe user do not what to migrate or what to trigger it manually with more control. I assume that was one of the idea to have a control app to do this?

danielhollas · 2023-10-17T09:07:58Z

Well for one we need to test that it indeed works before merging. Also the disadvantage is that this migration is hidden from the user. In this particular case, there is an extra message at the end of the migration with a command to recompute hashes of the calculation nodes due to a caching fix. We currently do not enable caching so it is not critical, but it is problematic that the user is completely unaware of this, unless they check the container startup logs. For the future, it would indeed be great if migration was explicitly triggered by the user I the control page imo. Dne út 17. 10. 2023 10:01 uživatel Jusong Yu ***@***.***> napsal:

…

I can not foresee any immediate issues, when I wrote this I was think about maybe user do not what to migrate or what to trigger it manually with more control. I assume that was one of the idea to have a control app to do this? — Reply to this email directly, view it on GitHub <#396 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACIY64M6VY2VIW4JMFGTIZDX7ZCPDAVCNFSM6AAAAAA2ERI2Z2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRVHE4DGNRVGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

yakutovicha · 2023-10-17T09:58:11Z

Sure, but why does it block this specific PR? This migration has been there for a while and never given problems (unless I am unaware). And we did go through database migrations in the past.

To be practical, I would suggest merging the PR (provided that tests are successful). Before releasing a new version, I could test the new image on dev-aiidalab server. If it fails, we stop everything and we look closely at the issue.

In the meantime, we can work on the proper user-controlled migration functionality.

To make myself clear: I do agree that this functionality needs to be improved, but not at the cost of blocking things.

danielhollas · 2023-10-17T10:28:39Z

@yakutovicha I don't think we had any migrations since the new Docker stack so I believe this warrants careful testing before merging.

Otherwise I agree there are no blockers.

unkcpz · 2023-10-18T20:30:33Z

The CI test failed for full-stack because that is the one who tries to install aiidalab-widgets-base and aiidalab-qe. It failed when installing aiidalab-qe I guess there are some dependency conflicts. Let's wait for the new test, I'll check it after.

danielhollas · 2023-10-20T18:04:31Z

@unkcpz one observation I made when looking at the test failure on GitHub (not directly related to this PR specifically): It seems that when the tests fail, the image is not published on ghcr.io as it used to before your refactoring. That makes debugging problematic since you can't download the built image to debug locally.

If my observation is correct, could you please move the image upload before the tests? Feel free to do it here or in a separate PR.

unkcpz · 2023-10-20T19:12:19Z

Good point, I didn’t consider this situation. I hope add an always() can solve the problem. I’ll do it here since there is a failed test I can test for.
Can you have a look at #412 if it is ready to be merge. Can save some build time to test things here.

danielhollas · 2023-10-20T22:12:05Z

I hope add an always() can solve the problem.

Yeah that should work as well (you can check the syntax in AWB where I used this to always upload screenshots as artifacts). But why do you think this solution would be better than simply reordering the steps?

unkcpz · 2023-10-21T15:04:45Z

But why do you think this solution would be better than simply reordering the steps?

Because the action that push the image to ghcr is in next action job not the next step (which is uploading the artifact to github).

danielhollas · 2023-10-21T16:46:53Z

Oh, okay. But what artifact is it uploading then? 😅

unkcpz · 2023-10-21T17:27:48Z

Seems works after add always(), the image is https://github.com/aiidalab/aiidalab-docker-stack/pkgs/container/full-stack/139750905?tag=amd64-pr-396

Oh, okay. But what artifact is it uploading then? 😅

It uploads the built image saved (by docker save), and here are the artifacts https://github.com/aiidalab/aiidalab-docker-stack/actions/runs/6598466593?pr=396
In fact, there is not need to go to ghcr and pull the image, but manually download the artifact which is a tar file, and run:

docker load --input /tmp/aiidalab/full-stack-amd64.tar

I personally am not a fan of pushing to ghcr, but since it was there I didn't change it during refactoring. It has two drawbacks:

It has to use GitHub token to push to the github package, therefore not for PR from a forked repository.
sha-xxx and pr-xxx tags in the ghcr registry make the ghcr registry a bit mass and potentially hide the useful tag to the user.

danielhollas · 2023-10-21T18:43:53Z

@unkcpz but uploading the artifact is the slow part no? That wasn't happening before, and I suspect if we simply push to ghcr.io straightaway it would be much more efficient. I don't think artifacts were designed for such large uploads, and I don't really see the benefits.

Even if you want to push to both places, you can do it in a single job, instead of having to download the artifact and upload it to ghcr, that's very inefficient

danielhollas · 2023-10-21T18:44:46Z

Also, normal users should pull from Docker, so the number of tags on ghcr.io is not a problem.

unkcpz · 2023-10-22T21:19:58Z

Back to the failed test itself, the issue related to aiidalab/aiidalab#397, I put here to cross link in case it is hard to find in the future.
The "stable" aiidalab-qe in the test is v23.04.6 which pining aiida-core with aiida-core~=2.3.0 and will downgrade the aiida-core to 2.3.1 and cause the installation failed for the following test cases.

In order to solve this problem I did two changes here:

towards core packages checking from pip-compile list aiidalab#397, I pining the lower bounded aiida-core with the installed version to prevent it downgraded from apps.
In the stable app version installation, I use --pre to use the released version without the tag-number version included.

unkcpz · 2023-10-22T21:34:36Z

Just found another issue in terms of aiidalab/aiidalab, after aiidalab/aiidalab@6671071, we didn't make any release and we remove the setup.py for both qeapp and your app. It leads to when install the app by aiidalab install the warning "WARNING:aiidalab.app:Warning: App 'quantum-espresso' does not declare any dependencies." pop up. I'll make a release for aiidalab/aiidalab. Since aiidalab uses calendar versioning, I think it contains quite some changes and deserves to be v23.10.0, what do you think @yakutovicha @danielhollas ?

(EDIT: after a second thought, maybe make a aiidalab release of v23.03.2 could be more safe)

danielhollas · 2023-10-22T22:32:11Z

(EDIT: after a second thought, maybe make a aiidalab release of v23.03.2 could be more safe)

I don't think there have been many changes so this one seems more appropriate. Please go ahead with the release.

Just found another issue in terms of aiidalab/aiidalab, after aiidalab/aiidalab@6671071, we didn't make any release and we remove the setup.py for both qeapp and your app.

Well, I did not if fact made this change, the PR for that is still open and marked as blocked. :-P ispg-group/aiidalab-ispg#183
I am a little surprised that you did not notice that issue, because that means you did not install QeApp for quite some time. Perhaps there should be an extra test in QeApp that checks the installation as well.

unkcpz · 2023-10-23T08:02:03Z

I did not if fact made this change, the PR for that is still open and marked as blocked.

Haha, I was too aggressive in the move. I found it was removed at aiidalab/aiidalab-qe@3971334, although the commit is only for remove this single file but hide in the large refactoring PR aiidalab/aiidalab-qe#439.

I am a little surprised that you did not notice that issue, because that means you did not install QeApp for quite some time.

We install the QeApp quite often, probably twice a day in my case. The problem is for development, it was installed using pip directly. In the docker image preparation, the aiidalab install is used in the end but the dependencies installation was taken of by pip explicitly and didn't break the app. Plus "WARNING:aiidalab.app:Warning: App 'quantum-espresso' does not declare any dependencies." was raised as a warning so didn't get my notice.

Two things I learned:

A specific test is needed in qeapp to test install by aiidalab install and check the app is all working well.
Specific purpose commit better to be separated as a PR (as @yakutovicha suggested before, maybe no need to have one commit per PR but for sure one purpose per PR).

unkcpz · 2023-10-23T08:05:45Z

Even if you want to push to both places, you can do it in a single job, instead of having to download the artifact and upload it to ghcr, that's very inefficient

I think we have been distracted a bit from various discussions here, I'll continue this discussion at #412 and hopefully, we can find the best solution and converge to a wiki post.

For bump the aiida-core==v2.4.0 I think it is ready for review.

danielhollas

@unkcpz thanks! I agree, that discussion should happen elsewhere.

I know this kind of contradicts the policy, but I would also suggest to publish a new aiidalab package version and bump it here. I looked at the commit history and besides the setup.py there are no functional changes. I would then do careful testing of everything (instead of having to test multiple times). Feel free to disagree though.

stack/base/Dockerfile

unkcpz · 2023-10-23T14:33:41Z

I would then do careful testing of everything (instead of having to test multiple times). Feel free to disagree though.

If there is a test by hand, I don't know what reason I can find to disagree with it . Always better to have a human test.
I bumped the aiidalab to 23.3.2, let's wait for the built and then test it. I'll do the test for the latest QEapp.

danielhollas · 2023-10-23T22:42:13Z

I've tested install of my app, both with and without setup.py. I will test the DB migration tomorrow.

danielhollas

I've tested the DB migration and things seems to work. @unkcpz once you test with QeApp I think this is good to go. 🚀

Here's how I tested:

Launch a fresh container with aiidalab/full-stack:latest image, with aiida-core=2.3.1
Install my app and run some calculations
Stop the container
Edit profile configuration to use aiidalab/full-stack:pr-396
Relaunch the container.
Check the container logs with aiidalab-launch logs -p <test_profile>
I saw this in the logs:

++ verdi storage migrate --force
Report: Migrating to the head of the main branch
Warning: Invalidating the hashes of certain nodes. Please run `verdi node rehash -e aiida.node:process.calculation.calcjob`.
Report: - main_0001 -> main_0002
Success: migration completed
++ verdi daemon start
Starting the daemon with 1 workers... OK

Verify in my app that the previous calculation is still there and that I can launch a new calculation without an issue.

danielhollas · 2023-10-24T15:50:04Z

tests/test-full-stack.py

@@ -5,7 +5,7 @@
 def generate_aiidalab_install_output(aiidalab_exec, nb_user):
    def _generate_aiidalab_install_output(package_name):
        output = (
-            aiidalab_exec(f"aiidalab install --yes {package_name}", user=nb_user)
+            aiidalab_exec(f"aiidalab install --yes --pre {package_name}", user=nb_user)


Not sure if this is needed, since we're also installing directly from the branch in test_install_apps_from_default_branch. But I guess it is fine.

Much appreciate for writing the detailed test flow, I'll test on QeApp with @superstar54 who has quite many containers with lots of calculations.

Per --pre options I added, it is for QeApp specifically since we didn't make the release v23.10.0 yet but the prerelease version. The stable version is still v23.04.6 which pinning to aiida-core~=2.3.0 will try to downgrade aiida-core and fail the test (exactly what we improved in this PR).

I personally think here it is better to not use --pre but only test the stable version. I will keep this PR until we make the release of v23.10.0 which the planned date is 6th November.

@unkcpz @superstar54 friendly ping, would be good to test and merge so that others can test this via the edge image.

Tested, all good. The migration is run and calculations can be reloaded with showing the same result properly.

unkcpz · 2023-11-29T21:35:40Z

We found a small issue with updating the pip to 23.3.1, where the fallback support using legacy setup.py install is removed, see pypa/pip#8368.
If an app version is not PEP 440 conformed, the installation was not failed but now it will fail.
The issue was reported by @AndresOrtegaGuerrero in MFA-CSCS app and fixed at aiidalab/aiidalab-mfa-cscs#14

I personally think we can keep on moving with the docker stacks and fix things on the app side, as the fixes I did in MFA-CSCS.

Bump aiida-core to v2.4.0

350d307

unkcpz force-pushed the bump/aiida-core-2.4.0 branch from aae8e4d to 350d307 Compare July 10, 2023 14:06

unkcpz force-pushed the main branch 4 times, most recently from 486feb1 to 0897d53 Compare July 10, 2023 23:18

unkcpz added this to the 2023.1014 milestone Jul 11, 2023

danielhollas marked this pull request as draft August 7, 2023 22:34

danielhollas mentioned this pull request Aug 31, 2023

Set caching as default for the profile config #407

Open

Merge branch 'main' into bump/aiida-core-2.4.0

1f77286

unkcpz requested a review from danielhollas October 16, 2023 12:24

unkcpz marked this pull request as ready for review October 16, 2023 12:24

Merge branch 'main' into bump/aiida-core-2.4.0

dafe020

test push testing failed image to ghcr

eba3da2

unkcpz force-pushed the bump/aiida-core-2.4.0 branch from b258a2f to eba3da2 Compare October 21, 2023 16:29

lower bound of aiida-core

08a0d7d

unkcpz force-pushed the bump/aiida-core-2.4.0 branch from fa6e066 to 08a0d7d Compare October 22, 2023 21:35

Revert CI docker for test changes

75045b5

danielhollas requested changes Oct 23, 2023

View reviewed changes

stack/base/Dockerfile Show resolved Hide resolved

stack/base/Dockerfile Outdated Show resolved Hide resolved

unkcpz added 2 commits October 23, 2023 12:56

f-da

2518fc0

bump pip to latest v23.3.1

5ff2408

unkcpz force-pushed the bump/aiida-core-2.4.0 branch from d845f42 to 5ff2408 Compare October 23, 2023 11:08

unkcpz changed the title ~~Bump aiida-core to v2.4.0~~ Bump aiida-core to v2.4.0 and aiidalab to v23.3.2 Oct 23, 2023

unkcpz added 2 commits October 23, 2023 16:29

bump aiidalab to v23.3.2

9d30415

Merge branch 'main' into bump/aiida-core-2.4.0

a2fd2c3

danielhollas self-requested a review October 24, 2023 15:47

danielhollas approved these changes Oct 24, 2023

View reviewed changes

unkcpz merged commit dfa6515 into main Nov 10, 2023
25 checks passed

unkcpz deleted the bump/aiida-core-2.4.0 branch November 10, 2023 10:59

unkcpz mentioned this pull request Nov 30, 2023

Bump pip from 23.0.1 to 23.3 in /stack/base #413

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump aiida-core to v2.4.0 and aiidalab to v23.3.2 #396

Bump aiida-core to v2.4.0 and aiidalab to v23.3.2 #396

unkcpz commented Jul 10, 2023 •

edited

Loading

danielhollas commented Jul 10, 2023 •

edited

Loading

unkcpz commented Jul 10, 2023

unkcpz commented Oct 16, 2023

danielhollas commented Oct 16, 2023

unkcpz commented Oct 16, 2023

yakutovicha commented Oct 17, 2023

unkcpz commented Oct 17, 2023

danielhollas commented Oct 17, 2023 via email

yakutovicha commented Oct 17, 2023

danielhollas commented Oct 17, 2023

unkcpz commented Oct 18, 2023

danielhollas commented Oct 20, 2023

unkcpz commented Oct 20, 2023

danielhollas commented Oct 20, 2023

unkcpz commented Oct 21, 2023

danielhollas commented Oct 21, 2023

unkcpz commented Oct 21, 2023 •

edited

Loading

danielhollas commented Oct 21, 2023

danielhollas commented Oct 21, 2023

unkcpz commented Oct 22, 2023

unkcpz commented Oct 22, 2023 •

edited

Loading

danielhollas commented Oct 22, 2023 •

edited

Loading

unkcpz commented Oct 23, 2023

unkcpz commented Oct 23, 2023

danielhollas left a comment

unkcpz commented Oct 23, 2023

danielhollas commented Oct 23, 2023

danielhollas left a comment

danielhollas Oct 24, 2023

unkcpz Oct 24, 2023

danielhollas Nov 9, 2023

unkcpz Nov 10, 2023

unkcpz commented Nov 29, 2023 •

edited

Loading

Bump aiida-core to v2.4.0 and aiidalab to v23.3.2 #396

Bump aiida-core to v2.4.0 and aiidalab to v23.3.2 #396

Conversation

unkcpz commented Jul 10, 2023 • edited Loading

danielhollas commented Jul 10, 2023 • edited Loading

unkcpz commented Jul 10, 2023

unkcpz commented Oct 16, 2023

danielhollas commented Oct 16, 2023

unkcpz commented Oct 16, 2023

yakutovicha commented Oct 17, 2023

unkcpz commented Oct 17, 2023

danielhollas commented Oct 17, 2023 via email

yakutovicha commented Oct 17, 2023

danielhollas commented Oct 17, 2023

unkcpz commented Oct 18, 2023

danielhollas commented Oct 20, 2023

unkcpz commented Oct 20, 2023

danielhollas commented Oct 20, 2023

unkcpz commented Oct 21, 2023

danielhollas commented Oct 21, 2023

unkcpz commented Oct 21, 2023 • edited Loading

danielhollas commented Oct 21, 2023

danielhollas commented Oct 21, 2023

unkcpz commented Oct 22, 2023

unkcpz commented Oct 22, 2023 • edited Loading

danielhollas commented Oct 22, 2023 • edited Loading

unkcpz commented Oct 23, 2023

unkcpz commented Oct 23, 2023

danielhollas left a comment

Choose a reason for hiding this comment

unkcpz commented Oct 23, 2023

danielhollas commented Oct 23, 2023

danielhollas left a comment

Choose a reason for hiding this comment

danielhollas Oct 24, 2023

Choose a reason for hiding this comment

unkcpz Oct 24, 2023

Choose a reason for hiding this comment

danielhollas Nov 9, 2023

Choose a reason for hiding this comment

unkcpz Nov 10, 2023

Choose a reason for hiding this comment

unkcpz commented Nov 29, 2023 • edited Loading

unkcpz commented Jul 10, 2023 •

edited

Loading

danielhollas commented Jul 10, 2023 •

edited

Loading

unkcpz commented Oct 21, 2023 •

edited

Loading

unkcpz commented Oct 22, 2023 •

edited

Loading

danielhollas commented Oct 22, 2023 •

edited

Loading

unkcpz commented Nov 29, 2023 •

edited

Loading