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

Minor refactoring of check_symlink #510

Merged
merged 3 commits into from
Mar 10, 2022

Conversation

muffato
Copy link
Contributor

@muffato muffato commented Mar 9, 2022

Hiya,

There are two changes in this pull-request.

The first two commits are a follow up of 10cf953. No need to call mkdirp for symlink_base since mkdirp is anyway called in create_symlink to create the parent directory. Then I slightly refactor check_symlink to give it a similar interface to create_symlink, i.e.: both are only called if the user wants symlinks, and neither need to check the symlink parameter.

The last commit is about stripping module.tcl from being shown by module avail in the symlinks tree. This is done by making the symlinks point at the module.tcl files directly, rather than their enclosing directories. It makes the symlink tree much shorter and nicer to use.
To achieve that, I had to change the module templates to hardcode the path to the modules, because otherwise it couldn't find the bin/ of the wrapper scripts.

Example trees:

containers/
├── golang
│   └── 1.18-rc
│       └── golang-1.18-rc-sha256:c9be2380bfdc0f4da95e1635e914dbd596bc73b618cdf16894a631eb58945156.sif
└── quay.io
    └── biocontainers
        ├── bwa
        │   └── 0.7.17--h7132678_9
        │       └── quay.io-biocontainers-bwa-0.7.17--h7132678_9-sha256:07822e4293a8c59755b295c448b9541db6c9bdbfdedb010bdbdcc1e1e935370f.sif
        ├── bwa-mem2
        │   └── 2.2.1--hd03093a_2
        │       └── quay.io-biocontainers-bwa-mem2-2.2.1--hd03093a_2-sha256:df046077d0771d622d2889039050a995a71430ed155f0f94f52e944bf2785b2a.sif
        ├── cooler
        │   ├── 0.8.11--pyh3252c3a_0
        │   │   └── quay.io-biocontainers-cooler-0.8.11--pyh3252c3a_0-sha256:c954780cf75fd8fe026de342253982a354b6211258e45f66e18d08af68271138.sif
        │   └── 0.8.6--py_0
        │       └── quay.io-biocontainers-cooler-0.8.6--py_0-sha256:1b0e2e9625d9d0beee5befaf866df016a80f364339fbb396ab8d84878a3a0584.sif
        └── samtools
            └── 1.15--h3843a85_0
                └── quay.io-biocontainers-samtools-1.15--h3843a85_0-sha256:d68e1b5f504dc60eb9f2a02eecbac44a63f144e7d455b3fb1a25323c667ca4c4.sif

modules/
├── golang
│   └── 1.18-rc
│       ├── 99-shpc.sh
│       ├── bin
│       │   ├── go
│       │   └── gofmt
│       └── module.tcl
└── quay.io
    └── biocontainers
        ├── bwa
        │   └── 0.7.17--h7132678_9
        │       ├── 99-shpc.sh
        │       ├── bin
        │       │   └── bwa
        │       └── module.tcl
        ├── bwa-mem2
        │   └── 2.2.1--hd03093a_2
        │       ├── 99-shpc.sh
        │       ├── bin
        │       │   └── bwa-mem2
        │       └── module.tcl
        ├── cooler
        │   ├── 0.8.11--pyh3252c3a_0
        │   │   ├── 99-shpc.sh
        │   │   ├── bin
        │   │   │   └── cooler
        │   │   └── module.tcl
        │   └── 0.8.6--py_0
        │       ├── 99-shpc.sh
        │       ├── bin
        │       │   └── cooler
        │       └── module.tcl
        └── samtools
            └── 1.15--h3843a85_0
                ├── 99-shpc.sh
                ├── bin
                │   ├── bgzip
                │   ├── htsfile
                │   ├── samtools
                │   └── tabix
                └── module.tcl

symlinks/
├── bwa
│   └── 0.7.17--h7132678_9 -> /lustre/scratch123/tol/teams/tolit/users/mm49/shpc/singularity-hpc/modules/quay.io/biocontainers/bwa/0.7.17--h7132678_9/module.tcl
├── bwa-mem2
│   └── 2.2.1--hd03093a_2 -> /lustre/scratch123/tol/teams/tolit/users/mm49/shpc/singularity-hpc/modules/quay.io/biocontainers/bwa-mem2/2.2.1--hd03093a_2/module.tcl
├── cooler
│   ├── 0.8.11--pyh3252c3a_0 -> /lustre/scratch123/tol/teams/tolit/users/mm49/shpc/singularity-hpc/modules/quay.io/biocontainers/cooler/0.8.11--pyh3252c3a_0/module.tcl
│   └── 0.8.6--py_0 -> /lustre/scratch123/tol/teams/tolit/users/mm49/shpc/singularity-hpc/modules/quay.io/biocontainers/cooler/0.8.6--py_0/module.tcl
├── golang
│   └── 1.18-rc -> /lustre/scratch123/tol/teams/tolit/users/mm49/shpc/singularity-hpc/modules/golang/1.18-rc/module.tcl
└── samtools
    └── 1.15--h3843a85_0 -> /lustre/scratch123/tol/teams/tolit/users/mm49/shpc/singularity-hpc/modules/quay.io/biocontainers/samtools/1.15--h3843a85_0/module.tcl

Example module avail output

----------------------------------------------- /nfs/users/nfs_m/mm49/nfs/scratch123/shpc/singularity-hpc/modules -----------------------------------------------
golang/1.18-rc/module.tcl                                    quay.io/biocontainers/cooler/0.8.6--py_0/module.tcl           
quay.io/biocontainers/bwa-mem2/2.2.1--hd03093a_2/module.tcl  quay.io/biocontainers/cooler/0.8.11--pyh3252c3a_0/module.tcl  
quay.io/biocontainers/bwa/0.7.17--h7132678_9/module.tcl      quay.io/biocontainers/samtools/1.15--h3843a85_0/module.tcl    

---------------------------------------------- /nfs/users/nfs_m/mm49/nfs/scratch123/shpc/singularity-hpc/symlinks -----------------------------------------------
bwa-mem2/2.2.1--hd03093a_2  bwa/0.7.17--h7132678_9  cooler/0.8.6--py_0  cooler/0.8.11--pyh3252c3a_0  golang/1.18-rc  samtools/1.15--h3843a85_0  

Best,
Matthieu

@vsoch
Copy link
Member

vsoch commented Mar 9, 2022

The first part of these changes look good! For:

To achieve that, I had to change the module templates to hardcode the path to the modules, because otherwise it couldn't find the bin/ of the wrapper scripts.

This is something @marcodelapierre and I just worked on to remove, meaning that the module installs were portable, and I'm hoping you can find a solution that can preserve that. If not, we can integrate the other changes and continue discussion on how to do that (for some future PR).

@muffato
Copy link
Contributor Author

muffato commented Mar 9, 2022

OK, I see. I could find the comments in the PRs. I'll see what I can do.

@marcodelapierre
Copy link
Contributor

marcodelapierre commented Mar 9, 2022

happy to help with the module path in the templates, just point me to the relevant line of code

@vsoch
Copy link
Member

vsoch commented Mar 9, 2022

So my understanding is that when we make a symlink tree:

$ shpc install --symlink-tree python
$ shpc use ./symlinks

To produce:

[root@528305e80a97 code]# tree symlinks/
symlinks/
`-- python
    `-- 3.9.10 -> /home/vanessa/Desktop/Code/shpc/modules/python/3.9.10

1 directory, 1 file

And then do module avail:

-------------------------------------------------------- symlinks --------------------------------------------------------
   python/3.9.10/module (D)

I think @muffato issue is that it shows module at the end? Or that the tree is slightly deeper than it could be otherwise? I guess I'm not fully understanding the problem or the need for a fix. If it's just an aesthetic thing but the fix takes a step backwards to remove the portability of the install I don't think it's a good direction to go in.

@muffato my suggestion for now, since you did state there are two changes here, is to:

  1. remove the hard coded module dir to leave your first set of fixes, and merge these fixes into Adding testing of a symlink tree install #502
  2. Review and merge both Adding testing of a symlink tree install #502 and bugfix for singularity pull (repeated output) and wrapper scripts template #506 since the latter has a bugfix (thank you!)
  3. Work on this additional issue in a fresh issue/discussion/pull request.

There are only three of us at the moment so we need to be organized and scoped with respect to how we review and make changes! I'm in full support of all discussion and changes but one thing at a time so we don't get overwhelmed :) Since we already have the symlink tree, and several bug fixes underway let's finish those up first! And let me know if I am misunderstanding and there is a bug/flaw with the current approach that must be addressed.

Let me know what you think!

@muffato
Copy link
Contributor Author

muffato commented Mar 10, 2022

It's mostly a cosmetic change as the module can still be loaded as per usual

Without my change, module avail print this:

bwa-mem2/2.2.1--hd03093a_2/module.tcl  cooler/0.8.6--py_0/module.tcl           golang/1.18-rc/module.tcl             
bwa/0.7.17--h7132678_9/module.tcl      cooler/0.8.11--pyh3252c3a_0/module.tcl  samtools/1.15--h3843a85_0/module.tcl  

With the change, module.tcl is stripped out:

bwa-mem2/2.2.1--hd03093a_2  bwa/0.7.17--h7132678_9  cooler/0.8.6--py_0  cooler/0.8.11--pyh3252c3a_0  golang/1.18-rc  samtools/1.15--h3843a85_0  

I agree with your plan, @vsoch

@vsoch
Copy link
Member

vsoch commented Mar 10, 2022

Sounds good! Ping me if you want any help, want to discuss things, or really whatever I'm here to help :)

@muffato muffato force-pushed the add/symlink-install branch from 2449aa1 to ed5a218 Compare March 10, 2022 00:26
@muffato muffato changed the title No module.tcl in the symlink tree Minor refactoring of check_symlink Mar 10, 2022
@muffato
Copy link
Contributor Author

muffato commented Mar 10, 2022

Done: this PR has now been restricted to the first two commits only

Co-authored-by: Vanessasaurus <814322+vsoch@users.noreply.github.com>
@vsoch
Copy link
Member

vsoch commented Mar 10, 2022

Awesome work @muffato ! let's get this merged into the symlink PR branch, and I suspect @marcodelapierre will take a look sometime in the coming weeks, and please open an issue if you want to discuss the tweaks to the symlink.

@vsoch vsoch merged commit 5bb4a06 into singularityhub:add/symlink-install Mar 10, 2022
@marcodelapierre
Copy link
Contributor

Hi @vsoch , @muffato , still working in limited capacity these weeks, so getting to these updates...slowly :)
I do have a comment on this one, but as I see it's now merged, I will comment in the appropriate PR

@vsoch
Copy link
Member

vsoch commented Mar 10, 2022

@marcodelapierre please no rush take whatever time you need! We'll be around :)

@muffato muffato deleted the add/symlink-install branch March 10, 2022 02:38
vsoch added a commit that referenced this pull request Jun 2, 2022
* adding testing of a symlink tree install
* unset symlink_home
* missing module load
* adding symlink_tree setting to indicate to always set
also adding support for custom config on the fly with -c and for
removing symlinks on uninstall
* missing command argument
* symlink_home -> symlink_tree and --symlink-tree instead of --symlink
* forgot to add dest
* ensure that we cleanup symlink directory of .version and empty
* ensure we dont create symlink version unless tcl and default version is true
* do not require symlink_base to be defined
we can have a default of $root_dir/symlinks instead
* bugfix: without this, create_symlink would only be called based on the CLI argument, not the .yml setting (#509)
* verison bump
* Minor refactoring of `check_symlink` (#510)
* No need to create the symlink base directory here since it will be created by `create_symlink`
* Make this part of the code symmetric with self.create_symlink()
* Fixed a truncated sentence
* running black
* Skip `module` in the symlinks (#511)
* Implemented "default_version" for TCL
* Use write_version_file for the symlink tree too
* Skip `module.tcl` in the symlinks
This is done by symlinking `<software>/<version>` itself to
`<namespace>/<software>/<version>/module.tcl`.
For the directory of the wrapper scripts to be correctly found, the
symlink has to be resolved, but TCL's `file normalize` won't normalise
the filename. So, we need to use `file readlink` instead, but only on
real symlinks because it raises an error.
* Symlink to module.lua when possible
which is when default_version==True (default_version==False can't be
made to work with symlinks).
* Added a `--force` option to `shpc install` to force overwriting existing symlinks
The name `--force` is generic, so that other things could be forced
through it, not just overwriting symlinks.
Also added an info message if a symlink is overwritten, which can be
hidden with the `--quiet` flag.
* Made `force` optional
* Forgot the variable for substitution
* The "delete" command was superseded by "uninstall" in #6
* Added `--no-symlink-tree` to override the config file
`--symlink-tree` now also overrides the config file
* Make it explicit we are expecting yes or no
* add force and symlink arg back in
* do not pin black
* Completion of the symlink feature (#539)
* Removed leftover from an earlier implementation of `default_version`
* bugfix: True and False are deprecated (but still valid)
* Lmod symlinks can now skip "module" too thanks to @marcodelapierre
* Added the missing newline character at the end of the file
* bugfix: the return needs to happen after the creation of the symlink
* `symlink_base` should allow environment variable expansion, like the other directories
* Cannot consider using symlinks without the base defined
* The two classes are meant to address exactly this
* bugfix: the symlink needs to be cleaned regardless of where the modules are held
* rmdir_to_base does the upwards clean-up correctly
* No need to complain if symlinks are not enabled in the first place
* bugfix: the caller of write_version_file needs to build the directory path
All other calls were already doing it, this was the only exception.
The change is required because not all .version files are in
$module_base
* bugfix: .version needs to be updated in the symlink tree too
* bugfix: when uninstalling the last version of a tool there is no directory any more
* Make the settings object upgrade legacy values so that the code only needs to know about the current ones
* Expanded the function to deal with files and symlinks
* Adding we can just use a filesystem loader and then from_string instead
* Message update
cf #502 (comment)
* saving start of work on views

this adds an shpc view command and refactors the symlink nomenclature to instead b
about views! I need to write documentation and make a few additional commands to load
a view from file and write tests for views too.
* adding changes to docs and tests for views!
* spelling mistake and missing tests helpers file
* force needs to be specific for uninstall
* remove deprecated symlink-tree
* adding support to install from a file and list installed modules
* legacy values is broken - remove for now
* fixing bug with module version files - cannot do any funny business with creating module class on the fly!
* module .version in symlink only should exist for lmod
* fixing bug with uninstall and adding shpc view list
* adding support to generate a view module
a user can now do shpc view add <var> <val> to add customizations to a view! In
practice this means that installed modules need to make an attempt to load the view
and given that it is a symbolic link, it should find the file (at least we hope). This
means in practice one view should be loaded at once, otherwise you will have two conflicting
.view_module.
* ensure we only use try-load for modules >= 4.8.

I am also updating the testing to run for both an older and newer version (with
and without support)

* fixing bug with test
I had changed the client variable to be module_sys instead of module to match
the rest of the library, and forgot to update it in the tests helper script.
* adding support for shpc view remove <name> <var> <value>

and I am also adding better testing to look at both the config and file
content of view_module in both cases

Signed-off-by: vsoch <vsoch@users.noreply.github.com>
Co-authored-by: vsoch <vsoch@users.noreply.github.com>
Co-authored-by: Matthieu Muffato <cortexspam-github@yahoo.fr>
Co-authored-by: Matthieu Muffato <mm49@sanger.ac.uk>
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