SwissDataScienceCenter / contributed-project-templates

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

🐛 Fix git-based user/email info for aiida profile #78

Closed GeigerJ2 closed 1 month ago

GeigerJ2 commented 3 months ago

This PR provides a first fix for the broken AiiDA template for RenkuLab.

Recently, trying to explore an AiiDA archive from Materials Cloud via RenkuLab created a Jupyter Notebook in which the provided code wasn't working due to a missing AiiDA profile. Manual profile creation via the shell would alleviate the issue, so the Docker image and AiiDA setup itself seem to be (in principle) still fine. As it turns out, though, the $GIT_AUTHOR_NAME and $EMAIL environment variables are unset, which is why the creation of the AiiDA profile on startup via the post-init.sh script fails. Now, the relevant information is obtained via git config, fixing the current problem.

However, the overall template is a bit outdated, using AiiDA v.1.6, and the aiida-activate tool, which has not been changed in the last 4 years. I have another PR in the pipeline that updates the template, i.e. using AiiDA v2.5, removing the aiida-activate dependency, and providing a new Docker image, but we're still testing some things. It'll come soon, though :)

rokroskar commented 3 months ago

@GeigerJ2 thanks for making these fixes! Could you give me a good archive_url to test this with?

GeigerJ2 commented 2 months ago

@GeigerJ2 thanks for making these fixes! Could you give me a good archive_url to test this with?

Not sure if you still need this, but I've been using:

https://archive.materialscloud.org/record/file?filename=acwf-verification_unaries-verification-PBE-v1_results_gpaw.aiida&record_id=1770

for testing.

GeigerJ2 commented 2 months ago

Thanks a lot @GeigerJ2 for looking into this! It's gone neglected for far too long :)

Unfortunately I think the string wrangling needs a second look - I get this in aiida_config.yaml:

...
first_name: "'Rok Roškar'"
last_name: "Rok Roškar'"
institution: Renkulab

Hm, that seems weird. There are both single quotes for first_name and only one for last_name? In any case, could you please give it a shot with the latest version on #79, and the URL I commented to see if there's an issue with the created AiiDA profile. In and of itself, AiiDA should handle special characters like "š" just fine (at least I hope so). Thanks!

rokroskar commented 2 months ago

Thanks a lot @GeigerJ2 for looking into this! It's gone neglected for far too long :) Unfortunately I think the string wrangling needs a second look - I get this in aiida_config.yaml:

...
first_name: "'Rok Roškar'"
last_name: "Rok Roškar'"
institution: Renkulab

Hm, that seems weird. There are both single quotes for first_name and only one for last_name? In any case, could you please give it a shot with the latest version on #79, and the URL I commented to see if there's an issue with the created AiiDA profile. In and of itself, AiiDA should handle special characters like "š" just fine (at least I hope so). Thanks!

Thanks @GeigerJ2 - how do I verify what was used for first_name last_name? When I do verdi profile show it doesn't show them.

GeigerJ2 commented 2 months ago

Thanks @GeigerJ2 - how do I verify what was used for first_name last_name? When I do verdi profile show it doesn't show them.

That's a good question. I thought it would be shown, but also for me, it doesn't. I really think it should, though. We're currently having some discussions about the internals of AiiDA profiles, so I'll bring that up for sure.

In the meantime, you could see the verdi profile setup command that was run via the pid file in the .aiida//access/aiida_renku directory, e.g.: /home/jovyan/work/user-name-test/aiida_data/.aiida/access/aiida_renku/18.pid

I just checked, and also in my case the first (last) name has an extra leading (trailing) '. So definitely something's going wrong in the post-init.sh script. Thanks for figuring this out, it might have been difficult to catch otherwise , without the yaml file! I'll resolve that tomorrow.

sphuber commented 2 months ago

Thanks @GeigerJ2 - how do I verify what was used for first_name last_name? When I do verdi profile show it doesn't show them.

Each profile can have multiple users with one being the default. You can show them with verdi user list.

sphuber commented 2 months ago

I am not quite sure how the code that parses the git username into a first and last name is supposed to work, but it seems not to be working as the first and last name both contain the entire name (including the problem with the quotes). I think the following should work as intended:

email="$(git config user.email)"
user="$(git config user.name)"
first_name=$(echo $user | cut -d ' ' -f1)  # Take all characters up until first space
last_name=$(echo $user | cut -d ' ' -f2-)  # Take all characters after the first space
cat > aiida_config.yaml <<EOF
profile: "default"
email: $email
first_name: $first_name
last_name: $last_name
institution: Renkulab
EOF

You don't need quotes around strings in YAML and think it might be part of the problem of the stray single quotes, but mostly this join_by function from stackoverflow seems to be culpable and simply not working.

GeigerJ2 commented 2 months ago

Yeah, frankly, I also didn't fully understand that. I just left it because I thought it was there for a reason. Didn't realize it's that broken, though :D But in the latest version, I also replaced it. It still uses a bash array as before, i.e.:

read -ra name_array <<< "$(git config user.name | tr -d "'")"
first_name="${name_array[0]}"
last_name="${name_array[@]:1}"

But indeed, using cut might be simpler. Too much bash syntax (e.g. arrays) gives me shivers.

I think the problem with the single quotes, however, is that running git config user.name fundamentally prints the name surrounded by single quotes in the shell on RenkuLab - which it doesn't do on any of the local systems I use. So I guess this ties in with the somewhat different behavior of git now as compared to when the template was first created, similar to the reason why it broke in the first place. I'll update the parsing to YAML here, using your snippet, then this should be ready for merge, and the full update hopefully soon, as well, when @rokroskar gets around to have a final look. Thanks!

rokroskar commented 1 month ago

@GeigerJ2 this is now already taken care of in #79, correct?

rokroskar commented 1 month ago

@GeigerJ2 if the changes here aren't needed lets close this and I will make a new release so that the new templates can be used by everyone.

GeigerJ2 commented 1 month ago

Yes, fine for me :)