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

Custom templates not substituted when the registry is local #588

Closed
muffato opened this issue Sep 4, 2022 · 11 comments
Closed

Custom templates not substituted when the registry is local #588

muffato opened this issue Sep 4, 2022 · 11 comments

Comments

@muffato
Copy link
Contributor

muffato commented Sep 4, 2022

This is the bug I was referring to in #586 (comment) . I've managed to reproduce it in a clear manner.

With a local registry, custom templates are not properly parsed and substituted. Instead, the resulting script only contains the path to the template. This can be seen with vanessa/salad from the official shpc registry

from a fresh clone of shpc on the main branch

$ shpc install vanessa/salad
Module vanessa/salad:latest was created.
$ head -n 1 modules/vanessa/salad/latest/bin/*
==> modules/vanessa/salad/latest/bin/fork <==
#!/bin/bash

==> modules/vanessa/salad/latest/bin/salad <==
#!/bin/bash

Both aliases are created and are bash scripts.

Now, let's start again with a clone of the registry

$ rm -rf modules/vanessa/
$ git clone https://github.com/singularityhub/shpc-registry registry_shpc
$ shpc config remove registry https://github.com/singularityhub/shpc-registry
$ shpc config add registry $PWD/registry_shpc
$ shpc install vanessa/salad
Module vanessa/salad:latest was created.
$ head -n 1 modules/vanessa/salad/latest/bin/*
==> modules/vanessa/salad/latest/bin/fork <==
/nfs/users/nfs_m/mm49/workspace/tol-it/central_software/singularity-hpc/registry_shpc/vanessa/salad/singularity_fork.sh
==> modules/vanessa/salad/latest/bin/salad <==
#!/bin/bash

fork is not a valid shell script anymore. It just contains the path of the template that should have been substituted.
salad is fine, because it was created from the default template.

@vsoch
Copy link
Member

vsoch commented Sep 4, 2022

Are you sure you just need to look at the whole file? E.g., when I head I see the same thing:

$ head -n 1 modules/vanessa/salad/latest/bin/*
==> modules/vanessa/salad/latest/bin/fork <==
#!/bin/bash

==> modules/vanessa/salad/latest/bin/salad <==
#!/bin/bash

But when I inspect the entire file, the content is indeed there.

$ cat modules/vanessa/salad/latest/bin/fork 
#!/bin/bash

script=`realpath $0`
wrapper_bin=`dirname $script`
moduleDir=`dirname $wrapper_bin`

singularity ${SINGULARITY_OPTS} exec ${SINGULARITY_COMMAND_OPTS} -B $moduleDir/99-shpc.sh:/.singularity.d/env/99-shpc.sh  /tmp/singularity-hpc/containers/vanessa/salad/latest/vanessa-salad-latest-sha256:e8302da47e3200915c1d3a9406d9446f04da7244e4995b7135afd2b79d4f63db.sif /code/salad fork "$@"

I cloned fresh from main as well. Let me know if I'm missing something!

@vsoch
Copy link
Member

vsoch commented Sep 4, 2022

Could you do a sanity check here:

if wrapper_script:
url = os.path.join(self.dirname, wrapper_script)
response = requests.get(url)
if response.status_code != 200:
logger.warning(
"Could not find wrapper script %s in remote registry." % script
)
return response.text
to see if the response is indeed 200,a and the text is correct? it could be the warning isn't showing and you are getting back some error message (or empty response text).

@muffato
Copy link
Contributor Author

muffato commented Sep 4, 2022

I used head to shorten the output. All scripts are meant to start with the shebang, and when it's missing something iss wrong.
The files that start with #!/bin/bash have more than 1 line on disk and are complete, whereas fork (from a local registry) has a single line.

@vsoch
Copy link
Member

vsoch commented Sep 4, 2022

Right, and I did the same commands as you from a fresh clone of main, and my fork (and salad) were complete, so something else is going on specific to your environment I think.

@muffato
Copy link
Contributor Author

muffato commented Sep 4, 2022

to see if the response is indeed 200,a and the text is correct? it could be the warning isn't showing and you are getting back some error message (or empty response text).

Why should this file be used at all when the only registry (as confirmed by shpc config get registry) is local ?

@vsoch
Copy link
Member

vsoch commented Sep 4, 2022

You’re right - it wouldn’t be! I was debugging the remote case, the first failure example you provided (which I just tried locally and everything shows up first).

Basically, if I can’t reproduce your issue I can try to give you suggestions for where to iPython.embed to look around. Since we know the templates aren’t working properly I would embed in the different spots to see how the wrapper template looks when it is retrieved (either a filename or content depending on remote or not) and how that gets rendered into the script in bin. Somewhere along that chain of events something is going wonky, and unfortunately I can’t reproduce so we need to debug and figure out what is different in your environment.

@muffato
Copy link
Contributor Author

muffato commented Sep 4, 2022

I could reproduce the error, exactly as in my first post, in a fresh container. Just put this in a Dockerfile and build it, and you'll end up with a "wrong" /root/modules/vanessa/salad/latest/bin/fork.

FROM python:3.8
ARG DEBIAN_FRONTEND=noninteractive
RUN apt-get update -y && apt-get install -y lmod build-essential libseccomp-dev pkg-config squashfs-tools cryptsetup
WORKDIR /tmp
ARG GO_VERSION=1.17.2
ARG GO_OS=linux
ARG GO_ARCH=amd64
RUN wget https://dl.google.com/go/go$GO_VERSION.$GO_OS-$GO_ARCH.tar.gz && \
    tar -C /usr/local --strip-components=1 -xzvf go$GO_VERSION.$GO_OS-$GO_ARCH.tar.gz && \
    rm go$GO_VERSION.$GO_OS-$GO_ARCH.tar.gz
ARG SING_VERSION=3.9.5
RUN wget https://github.com/sylabs/singularity/releases/download/v${SING_VERSION}/singularity-ce-${SING_VERSION}.tar.gz && \
    tar -xzf singularity-ce-${SING_VERSION}.tar.gz && \
    cd singularity-ce-${SING_VERSION} && \
    ./mconfig && \
    make -C builddir && \
    make -C builddir install && \
    cd .. && rm -rf singularity-ce-${SING_VERSION} singularity-ce-${SING_VERSION}.tar.gz
WORKDIR /root
RUN git clone -b 0.1.11 https://github.com/singularityhub/singularity-hpc.git
WORKDIR singularity-hpc
RUN pip install -e .[all]
RUN shpc install vanessa/salad
RUN head -n 100 modules/vanessa/salad/latest/bin/*
RUN rm -rf modules/vanessa/
RUN git clone https://github.com/singularityhub/shpc-registry registry_shpc
RUN shpc config remove registry https://github.com/singularityhub/shpc-registry
RUN shpc config add registry $PWD/registry_shpc
RUN shpc install vanessa/salad
RUN head -n 100 modules/vanessa/salad/latest/bin/*

@vsoch
Copy link
Member

vsoch commented Sep 4, 2022

Perfect thank you! Love containers!

@vsoch
Copy link
Member

vsoch commented Sep 5, 2022

okay reproduced! Testing in wrappers/base.py, it looks like there is content returned with a path (which should not happen)

In [4]: result
Out[4]: {'content': '/root/singularity-hpc/registry_shpc/vanessa/salad/singularity_fork.sh'}

The reason is because the function to the config to load the wrapper script is returning the file

            wrapper_script = self.config.load_wrapper_script(
                self.container.command, self.wrapper_template
            )

which is calling the underlying provider to get it (and returning the wrong thing still)

self.entry
Out[3]: <shpc.main.registry.filesystem.FilesystemResult at 0x7fcb5af6bd60>

In [2]: self.entry.load_wrapper_script(container_tech, script)
Out[2]: '/root/singularity-hpc/registry_shpc/vanessa/salad/singularity_fork.sh'

and all this nonsense is because of a careless typo! Probably by me :)

    def load_wrapper_script(self, container_tech, script):
        """
        Get the content of a wrapper script, if it exists.
        """
        wrapper_script = self.find_wrapper_script(container_tech, script)
        if wrapper_script:
-            return os.path.join(self.dirname, wrapper_script)
+            return shpc.utils.read_file(os.path.join(self.dirname, wrapper_script))

Now I'm not sure why this was happening for the remote too for you - that should have triggered the same function but retrieved content from a web request. We will want to check that out too after I push a fix!

And here it is! 41a3991#diff-b77a2993a3eb156f02a3718b1b243c442c3d98988843473675a8fe45f7798e7e

@muffato
Copy link
Contributor Author

muffato commented Sep 5, 2022

All good, that's solved the issue ! There was no problem when the registry was remote

@muffato muffato closed this as completed Sep 5, 2022
@vsoch
Copy link
Member

vsoch commented Sep 5, 2022

Oh, doh! That's why (at first shot) I couldn't reproduce - I read your issue wrong that it was present for both (and only tested the remote assuming it would show up). Sorry for the extra work - but the reproduci-tainer was great!

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

No branches or pull requests

2 participants