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

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

Merged

Conversation

GeigerJ2
Copy link
Contributor

@GeigerJ2 GeigerJ2 commented Mar 15, 2024

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

  • The Dockerfile is updated, and a new image pinned in renku.ini (using the latest renku 0.22 Python 3.10 base
    image and an AiiDA v2.5.1 installation)
  • aiida-activate was removed as the repo is outdated
  • Instead, the AiiDA profile is set up via verdi profile setup in the post-init.sh script, where depending on the presence (absence) of the archive_url the sqlite_zip (sqlite_dos) backends are used
  • Currently, the AiiDA Postgres backend (psql_dos) cannot be used due to missing sudo access → For inspection sqlite_zip and sqlite_dos should be sufficient (frankly, I tried resolving that using the aiida-core Docker images/Dockerfiles for some time, but didn't manage to make it work, and the effort might not be worth it if we stick to the other backends)
  • RabbitMQ is working as expected as it is installed via the aiida-core.services conda package, the consumer_timeout variable is set, and the version warning disabled
  • Minor changes to the explore notebook and the README

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
Copy link

sphuber commented Mar 15, 2024

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
Copy link
Contributor Author

GeigerJ2 commented Mar 15, 2024

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
Copy link
Contributor Author

GeigerJ2 commented Apr 5, 2024

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

@ltalirz
Copy link
Contributor

ltalirz commented Apr 5, 2024

I was in contact with @rokroskar when this started

@rokroskar
Copy link
Member

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

@rokroskar
Copy link
Member

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

Copy link
Member

@rokroskar rokroskar left a comment

Choose a reason for hiding this comment

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

Thanks @GeigerJ2! Some minor comments.

As mentioned in one of my comments - why not include the changes from #78 here?

aiida/environment.yml Outdated Show resolved Hide resolved
aiida/post-init.sh Outdated Show resolved Hide resolved
aiida/requirements.txt Show resolved Hide resolved
@rokroskar
Copy link
Member

btw there is also #8 which would be great to either wrap up or close :)

@GeigerJ2
Copy link
Contributor Author

GeigerJ2 commented Apr 9, 2024

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 GeigerJ2 force-pushed the fix/update_aiida_template branch 4 times, most recently from c94a469 to 96082ff Compare April 15, 2024 16:57
@GeigerJ2
Copy link
Contributor Author

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 #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!

Copy link
Member

@rokroskar rokroskar left a comment

Choose a reason for hiding this comment

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

Thanks @GeigerJ2! Just two more small things I noticed...

aiida/post-init.sh Outdated Show resolved Hide resolved
aiida/post-init.sh Outdated Show resolved Hide resolved
@GeigerJ2
Copy link
Contributor Author

GeigerJ2 commented Apr 17, 2024

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

@GeigerJ2
Copy link
Contributor Author

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

@rokroskar
Copy link
Member

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 GeigerJ2 closed this May 13, 2024
@GeigerJ2 GeigerJ2 force-pushed the fix/update_aiida_template branch from e586751 to d6e2fa3 Compare May 13, 2024 09:32
@GeigerJ2 GeigerJ2 reopened this May 13, 2024
@GeigerJ2 GeigerJ2 force-pushed the fix/update_aiida_template branch 3 times, most recently from 8cab7d4 to e36f7f2 Compare May 13, 2024 10:12
@GeigerJ2
Copy link
Contributor Author

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
Copy link
Member

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

@GeigerJ2
Copy link
Contributor Author

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
Copy link
Member

Yep, that's fine!

@rokroskar rokroskar changed the title ⬆️ Update AiiDA version, docker, and post-init.sh chore: update AiiDA version, docker, and post-init.sh May 13, 2024
@rokroskar rokroskar changed the title chore: update AiiDA version, docker, and post-init.sh fix: update AiiDA version, docker, and post-init.sh May 13, 2024
@rokroskar rokroskar added this pull request to the merge queue May 13, 2024
Merged via the queue into SwissDataScienceCenter:main with commit 183f123 May 13, 2024
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.

4 participants