SwissDataScienceCenter / contributed-project-templates

A repository of community-developed project templates.
Apache License 2.0
4 stars 15 forks source link

fix: update AiiDA version, docker, and post-init.sh #79

Closed GeigerJ2 closed 1 month ago

GeigerJ2 commented 3 months ago

Fix and update the RenkuLab AiiDA template

This PR updates the AiiDA template for RenkuLab.

Regarding the broken profile creation on start-up, see PR #78.

Main changes

Some further notes

Currently, the directory where the archive is downloaded is "repo", but one could also use the existing "data" directory instead? I think this ties in with our discussion about verdi init, @sphuber and @giovannipizzi. If we manage to merge these changes soon, we could update the setup here - for now it works in the classical way. Pinging @ltalirz and @khsrali here for info as well.

If somebody wants to take it for a spin before the merge, you can provide my repository URL and the latest commit SHA of this PR when creating a new RenkuLab project, and fetch the updated template (see screenshot below).

Would be happy about feedback or suggestions for improvement. We also had the idea of providing some additional custom code, which already gives some interesting information about the archive (e.g. as a pandas dataframe), though, I think this should be as agnostic of the structure of the data as possible and would warrant a separate PR either way.

Lastly, when trying to import this archive after the notebook is created (that is, not providing an archive_url initially), I'm getting a NotImplementedError (see screenshot below), while pure inspection via sqlite_zip without importing works. I haven't investigated it further so far, but could this be possibly a case in which an automatic migration is required, @superstar54?

Creating a project from the template

image

NotImplementedError on archive import

image

sphuber commented 3 months ago

Lastly, when trying to import this archive after the notebook is created (that is, not providing an archive_url initially), I'm getting a NotImplementedError (see screenshot below), while pure inspection via sqlite_zip without importing works. I haven't investigated it further so far, but could this be possibly a case in which an automatic migration is required, @superstar54?

This is expected. The sqlite_zip backend is read only. When you are importing, you are trying to add new data to the storage, which is not supported. When you create a profile using sqlite_zip for an existing archive, you are essentially just "mounting" it as a read-only storage, you are not adding anything to it. So you either create an empty sqlite_dos profile and then import the archive, or you use sqlite_zip that just mounts the archive.

GeigerJ2 commented 3 months ago

This is expected. The sqlite_zip backend is read only.

When no archive_url is provided, the profile is initialized with sqlite_dos, though, to enable archive import further down the line. jinja gets evaluated when the RenkuLab project is created from the template, so I'm checking if archive_url is set and depending on that, the post-init.sh script is set up accordingly:

https://github.com/GeigerJ2/contributed-project-templates/blob/8dfb01bf9543a2d0a3268b2b8357c6135fe9536b/aiida/post-init.sh#L48-L60:

{% else %}

# Without archive_url, generate profile using `core.sqlite_dos` backend
verdi profile setup core.sqlite_dos \
    --profile $aiida_profile \
    --first-name "$first_name" \
    --last-name "$last_name" \
    --email "$email" \
    --institution RenkuLab \
    --set-as-default \
    --non-interactive

{% endif %}

I updated the screenshot in my original PR message so that it now actually shows the backend, making the issue clearer.

GeigerJ2 commented 3 months ago

Just a ping if one of the maintainers can have a quick look here? Thanks!

ltalirz commented 3 months ago

I was in contact with @rokroskar when this started

rokroskar commented 3 months ago

Ah thanks for pinging me @ltalirz I missed this PR! Thank you @GeigerJ2 I'll have a look asap!

rokroskar commented 3 months ago

btw, for quickly creating a project with this PR, you can use this link.

rokroskar commented 3 months ago

btw there is also https://github.com/SwissDataScienceCenter/contributed-project-templates/pull/8 which would be great to either wrap up or close :)

GeigerJ2 commented 3 months ago

Thanks, @ltalirz, for the ping to @rokroskar, and thanks to you for getting back quickly. I'll take care of your comments here, and on #78 in the following days. This is just to confirm that I'm aware of them :)

GeigerJ2 commented 2 months ago

I addressed your changes in my recent commit. Along the way, I tried to restrict the installation to only using conda, and pin both aiida-core and aiida-core.services to the latest v2.5.1, but as seen in my comment above this was not straightforward. Another possible improvement I tried out was to install everything into a dedicated aiida_renku conda environment, rather than base, but the notebook wouldn't pick that up, so I reverted it. Using base should be fine for now.

btw there is also https://github.com/SwissDataScienceCenter/contributed-project-templates/pull/8 which would be great to either wrap up or close :)

I don't have any opinion on that change. If nothing speaks against it, then I could add it here. 0.25 cpu seems indeed a bit small :D so you let me know. Thanks again, @rokroskar!

GeigerJ2 commented 2 months ago

Thanks again for looking into it @rokroskar! Is this ready to be merged, or are other changes required?

GeigerJ2 commented 2 months ago

Pinging @rokroskar, as I'd like to close this soon. Thank you! :)

rokroskar commented 1 month ago

Apologies @GeigerJ2 I didn't get to this until now - I just tested the project and it looks good. I still think you might want to consider pinning the image you specify in renku.ini (i.e. not use latest) but you can also do this later if you want. Please resolve the conflict in the Dockerfile and then I can approve.

GeigerJ2 commented 1 month ago

Hi @rokroskar, thanks for getting back here. I updated the branch via an interactive rebase, cleaning up the commit history a bit. I found the latest tag to be quite convenient, but I'm also happy with pinning it via the hash. Could you please remind me again which part of which hash I have to use here (checked here and here, but find the relevant information). I tried with the first and last seven characters of the commit hash on the GitLab repo that hosts the image, as well as the image digest, but seems like these are not the correct specifications? Thanks!

rokroskar commented 1 month ago

hi @GeigerJ2 - you'll need to uncomment these lines for the CI job to build the image.

GeigerJ2 commented 1 month ago

hi @GeigerJ2 - you'll need to uncomment these lines for the CI job to build the image.

Ah, alright, I see. Though, the project on GitLab is now outdated compared to the state of the code here, as I just regularly built the image locally and pushed it to the registry. Is it fine for now to keep the latest tag and get this merged so that the link to Renku on Materials Cloud Archive works again? I'll update it properly once we release a new AiiDA version, as that will also contain useful new features for the Renku integration.

rokroskar commented 1 month ago

Yep, that's fine!