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

Add Guix buildpack #1048

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add Guix buildpack #1048

wants to merge 2 commits into from

Conversation

hulecomte
Copy link

Hi! This patch adds a backend that uses the GNU Guix package manager. Guix provides reproducible software environments, transparency and provenance tracking. Coupled with a channels.scm file, it allows users to pin to a specific Guix revision from which the set of packages listed in their manifest.scm is deployed, thereby supporting very fine-grain reproducibility.

Let me know what you think

@welcome
Copy link

welcome bot commented Jun 21, 2021

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

Copy link
Collaborator

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

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

Thank you so much for adding this.

We already support nix here - can you tell me just a little bit about how they relate to each other?

Also, the current maintainers of repo2docker don't have a lot of guix experience. Do you think there will be enough interest from the guix community itself to keep this up to date?

I've left general comments but haven't managed to test it.

I look forward to working on getting this merged!

repo2docker/buildpacks/guix/__init__.py Outdated Show resolved Hide resolved
repo2docker/buildpacks/guix/__init__.py Outdated Show resolved Hide resolved
@civodul
Copy link

civodul commented Jul 5, 2021

Hi @yuvipanda. I can reply on the Nix vs. Guix bit (I'm a Guix co-maintainer). :-)

Guix and Nix are independent projects with different code bases. The tools are both "functional package managers", which sets them apart from other tools out there; you'll find some of the core concepts are the same, and Guix borrowed them from Nix (Guix's build daemon is based on that of Nix).

Apart from that, the user interfaces are different (in Guix we try hard to make the CLI accessible to non-experts, to offer customization options, and to provide good integration for a variety of software deployment tools), the packages they provide are different (e.g., Guix insists on free software built from source as a way to improve transparency and security), and so on.

There's a growing scientific computing community around Guix with a focus on high-performance computing (HPC) and reproducible science. I'm confident there'll be interest in keeping Guix support afloat in repo2docker.

Let me know if you need more info!

@yuvipanda
Copy link
Collaborator

@civodul Thank you for your explanations - that sounds good enough to me. Let's working on getting stuff outside of $HOME and work towards getting this deployed! \o/

If you rebase your PR, it should also fix the failing docker build and build docs as well.

@hulecomte
Copy link
Author

Thanks for your feedback @yuvipanda, what do you think now ?

Copy link
Collaborator

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

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

Thanks, @hulecomte! I've left some more comments. I haven't tested it locally yet, though.

Another question I have is - if a user sets up a repo to use guix, then they will need to make sure that jupyter is installed inside the container. the Nix buildbpack has a nice note, can you add something similar for Guix too?

I'm not sure why, but it looks like the lint failure must be fixed first before the integration tests will run (not sure why?!). Can you take a shot at it?

Thank you for all your work! \o/

repo2docker/buildpacks/guix/guix-install.bash Outdated Show resolved Hide resolved
repo2docker/buildpacks/guix/guix-install.bash Outdated Show resolved Hide resolved
repo2docker/buildpacks/guix/__init__.py Outdated Show resolved Hide resolved
assemble_script ="""
/var/guix/profiles/per-user/root/current-guix/bin/guix-daemon \
--build-users-group=guixbuild --disable-chroot & \
mv /usr/bin/python /usr/bin/python.debian && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add some more comments on why this is necessary? Removing system python might have unintended side effects, no? Wouldn't the path precedence deal with it?

Copy link
Author

Choose a reason for hiding this comment

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

I added some comments about this steps in the header of the function, let me now what you think.

The point of using Guix is to avoid any confusions about the version or the provenance of the software you want to use and therefore guarantee and allow reproducibility of a software environment. Therefore having a python that we now nothing about could jeopardize that. Also counting on the path precedence to deal with that give us no guarantee of software environment reproducibility and could influence our results by making things work when they shouldn't for example.

While testing the Guix buildpack, I tried installing and using numpy, it take me a long time to realize that I where trying to do so but with the python already installed on the docker image, wich could not work with numpy (versions conflict). Also it can be confusing when the software use environment variables and you are not aware of all the other software present on the system.

repo2docker/buildpacks/guix/__init__.py Outdated Show resolved Hide resolved
repo2docker/app.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@yuvipanda yuvipanda 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 addressing these, @hulecomte! I still haven't managed to test this locally, but hope this is all still useful.

To respond to #1048 (comment), IMO we shouldn't hand-move things installed with another package manager (in this case, apt). If you try to use apt to remove it, I'm sure it'll take a bunch of other packages with it. IMO, this means we shouldn't try to do that, since it will just silently break other things. We already do this with nix, conda, etc - IMO that's what we should do here too.

When the container runs and the user tries to do pip install numpy, it should automatically use the 'correct' python installed - in this case, the guix one. For example, in the conda buildpack (

path.insert(0, "${CONDA_DIR}/bin")
), just the PATH is enough to 'do the right thing'. And /usr/bin/python3 is never used.

Let's try do the same here?

Thanks for working with me through this!

@@ -0,0 +1,13 @@
# This downloads and installs a pinned version of Guix using a pinned version of the installation script.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# This downloads and installs a pinned version of Guix using a pinned version of the installation script.
#!/bin/bash
# This downloads and installs a pinned version of Guix using a pinned version of the installation script.

Copy link
Author

Choose a reason for hiding this comment

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

Hey thanks for that, I will add it in the next rebase!

"""
assemble_script ="""
/var/guix/profiles/per-user/root/current-guix/bin/guix-daemon \
--build-users-group=guixbuild --disable-chroot & \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this single & intentional? Does this daemon continue to run in the background after this step is done?

Copy link
Author

Choose a reason for hiding this comment

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

Indeed, you need the guix-daemon running to be able to use guix!

/var/guix/profiles/per-user/root/current-guix/bin/guix-daemon \
--build-users-group=guixbuild --disable-chroot & \
mv /usr/bin/python /usr/bin/python.debian && \
su - $NB_USER -c '{}' && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can return multiple scripts from here, some to be run as root and some as NB_USER. That's probably a bit cleaner than using su here

Copy link
Author

Choose a reason for hiding this comment

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

Hey, so Im struggling a bit with this part. It seems I can't keep the guix-daemon running between distinct steps. In some cases I have some issues when the docker images is not completely rebuilt, steps by steps but reusing some steps already executed before. That's why I did that here, I launch the guix-daemon just before running the Guix command I need, therefore I'm absolutely sure guix-daemon is running. Maybe you have some clues or ideas about a better way to do so ?

@hulecomte
Copy link
Author

Thanks for addressing these, @hulecomte! I still haven't managed to test this locally, but hope this is all still useful.

To respond to #1048 (comment), IMO we shouldn't hand-move things installed with another package manager (in this case, apt). If you try to use apt to remove it, I'm sure it'll take a bunch of other packages with it. IMO, this means we shouldn't try to do that, since it will just silently break other things. We already do this with nix, conda, etc - IMO that's what we should do here too.

When the container runs and the user tries to do pip install numpy, it should automatically use the 'correct' python installed - in this case, the guix one. For example, in the conda buildpack (

path.insert(0, "${CONDA_DIR}/bin")

), just the PATH is enough to 'do the right thing'. And /usr/bin/python3 is never used.

Let's try do the same here?

Thanks for working with me through this!

Hey yuvipanda! I'm currently trying that but it seems I still can use python while using bash script to test the buildpack...Maybe I am not doing it well enough? What do you think ?

Anyway, I don't now why lint test is failing and I don't know how it works too. How can I reproduce it locally ?

Thanks!

@hulecomte hulecomte requested a review from yuvipanda July 30, 2021 12:13
@hulecomte
Copy link
Author

hulecomte commented Aug 11, 2021

Hey @yuvipanda here are some updates:
-I modified the doc and the guix-install.bash as you suggested.
-Added a PATH 'do to the right thing' as we discussed it previously.

What do you think?

@civodul
Copy link

civodul commented Sep 17, 2021

Hi @yuvipanda & all! Would be great if @hulecomte's work could get in. :-)

Is there anything left that you think we should do?

Thanks in advance!

@civodul
Copy link

civodul commented Jan 18, 2022

Hi @yuvipanda, could you take another look at this PR?

Thanks in advance.

@rekado
Copy link

rekado commented Feb 1, 2022

I'm very excited about this addition. Are there any other obstacles to getting this merged?

@consideRatio consideRatio changed the title [MRG] Add Guix buildpack Add Guix buildpack Oct 30, 2022
@consideRatio consideRatio reopened this Oct 31, 2022
@civodul
Copy link

civodul commented Nov 3, 2022

Hi! Any update here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants