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

update aiida project template #4

Merged
merged 13 commits into from
May 4, 2021

Conversation

ltalirz
Copy link
Contributor

@ltalirz ltalirz commented Apr 13, 2021

Updates to the AiiDA project template

  • disable data import on environment launch
  • add jupyter notebook that opens on environment launch
    (with instructions on how to import & work with the data)
  • various updates and bug fixes

Questions:

  • @rokroskar Is there a way to set environment variables on the container level? I'm setting one (AIIDA_PATH) in the .bashrc and it is available in the terminal, but not in the jupyter notebook.

Todo:

  • (optional, to decide) set up AiiDA line magic

 * update to latest renkulab dockerfile
 * update aiida-core python requirement
Add jupyter notebook that allows to
 * import the dataset
 * explore it
when run locally, project name may be empty (and path may be different).
aiida-activate check for profile existence is fragile
@rokroskar
Copy link
Member

Hi @ltalirz thanks for this update! Regarding your first question have you tried adding an export VAR=val to the post-init script?

@ltalirz
Copy link
Contributor Author

ltalirz commented Apr 15, 2021

Thanks - for some reason I didn't think of trying this.

I've modified the post-init.sh here:
https://renkulab.io/gitlab/leopold.talirz1/mc-import4/-/blob/master/post-init.sh#L39

But it turns out that doesn't propagate to the environment in the terminal
image

(same in the jupyter notebook)

@rokroskar
Copy link
Member

Ah you're right of course - I thought the script was sourced but it's actually executed with bash. I think if we sourced it instead it would (probably) work...

@ltalirz
Copy link
Contributor Author

ltalirz commented Apr 15, 2021

It would be quite useful to be able to set environment variables, so I would support this.

@rokroskar
Copy link
Member

Ok I will make an issue for it - I won't be able to get to it today, most likely...

@ltalirz
Copy link
Contributor Author

ltalirz commented Apr 28, 2021

@rokroskar this should be ready for review now. I tested it and it seems to work fine.

P.S. By the way, I again get the warning about an outdated renku version, but it displays the same version for the new one and the one of the repo
image

Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
A minor note: we have a new renku-python release, you should probably include it in the template (see inline).

I tried to run the code on the notebook but I got an error, and I noticed the daemon is not running. Is this expected (i.e. I need to configure something first) or should it be running anyway?
Screenshot_20210504_093046

aiida/Dockerfile Outdated Show resolved Hide resolved
@ltalirz
Copy link
Contributor Author

ltalirz commented May 4, 2021

thanks for the review @lorenzo-cavazzi !

I tried to run the code on the notebook but I got an error,

Would you mind sharing the error you saw? I remember trying it and it did work fine for me.

It may have been that you did not specify a path to an AiiDA archive file (although that field was optional, part of the template did assume that a valid URL was provided). For an example URL see #4 (comment)

The archive URL will be prefilled for users that enter via the Materials Cloud Archive, but of course the template should still work if someone creates a project from it manually and does not specify the url. I've now put in some extra guards to also cover that use case.

@ltalirz
Copy link
Contributor Author

ltalirz commented May 4, 2021

P.S. Yes, it is expected that the daemon is not running - I forgot to update the expected output in the README (now fixed).

@lorenzo-cavazzi
Copy link
Member

After your commits, I don't get the exception anymore.
It's probably not relevant, but here is the error I got before:

Screenshot_20210504_140413

Currently, if I don't provide the initial URL, it fails gracefully 👍

Screenshot_20210504_140435

There is another minor detail on the README.md to possibly adjust, I'll provide the details in a commit suggestion. Fell free to include or discard it, then we are ready to merge this PR.

I have one final question: I noticed the repository contains uncommitted changes when I access a pristine environment. I assume a post initialization script runs something that creates those files.
Would It make sense to exclude the repo folder using the .gitignore file? Or adding some logic to the script to run also a git commit when the new files are created?
I'm not familiar with this system, so disregard my comment if the current behavior is the expected one.

@ltalirz
Copy link
Contributor Author

ltalirz commented May 4, 2021

I have one final question: I noticed the repository contains uncommitted changes when I access a pristine environment. I assume a post initialization script runs something that creates those files.
Would It make sense to exclude the repo folder using the .gitignore file? Or adding some logic to the script to run also a git commit when the new files are created?

Thanks! The repo folder contains the internal provenance tracking of AiiDA which I would not suggest to commit to git for the time being (for this one should think about a deeper integration between renku and AiiDA first) - however, I would prefer if users can restore it when they come back to the environment.

Will adding it to the .gitignore also lead to the contents of the folder being lost when the environment is shut down?
If yes, I suggest to keep it as is.

aiida/README.md Outdated Show resolved Hide resolved
@lorenzo-cavazzi
Copy link
Member

The code won't be preserved when starting a new environment from a new commit.
I personally believe it's very easy that users may add that folder by mistake to a commit if it's not ignored, but I have nothing against leaving it as is if you think it's fine. That can also be changed later if needed.

@ltalirz
Copy link
Contributor Author

ltalirz commented May 4, 2021

Ok!

I'm sure there will be further changes once people start using it, and we'll try to make it as user-friendly as possible.

Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

Let's merge this!

@lorenzo-cavazzi
Copy link
Member

I've tagged a new version here and included the reference in the main renku repository, so that the new templates will be included in renkulab.io on the next release SwissDataScienceCenter/renku#2114 .

If this is urgent, let us know and we can speed up the process

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.

None yet

3 participants