-
-
Notifications
You must be signed in to change notification settings - Fork 26
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
Adding testing of a symlink tree install #502
Conversation
Signed-off-by: vsoch <[email protected]>
Signed-off-by: vsoch <[email protected]>
Signed-off-by: vsoch <[email protected]>
Signed-off-by: vsoch <[email protected]>
So, as regards the symlink tree, this sounds like a great idea indeed! To be honest, I was planning to generate this type of compact tree using a bash script on our HPC centre, so....certainly handy if SHPC can handle the creation of a more compact moduletree. And I really like @muffato 's key idea of having SHPC generating a duplicate, symlinked tree, and warn users when overwriting occurs. I haven't got the chance (and won't for a while) to have a look at the code edits of this PR, but here are Some thoughts:
that's it for now :) |
Oh those are some good points on the cleanup / uninstall - what we would do is generate the same symlink path and then remove it if it exists. if the user changes their symlink_home then it would be up to them to nuke / manage the whole thing. I'm only up briefly but here are some thoughts!
I like those names but I'm worried it doesn't make it stupidly obvious what we've created. It's a "symlink tree" or a view of the main modules enabled by symlinks. A module shortcut or compact module I'm not sure I would know it's a symlink.
I am thinking of it a bit like a view, and in which case I might actually want to manage the tree for my users, maybe not always providing all containers or tags in the view but some select set. So my thinking is we probably want to empower all use cases - being able to install a new moduble and say "add to the symlink tree" on the fly, but perhaps ALSO have a global flag in the settings to say "do this by default." I can add this tomorrow evening - I'll add:
to settings.yml and that will be equivalent to always providing the
This could get very complicated very fast, e.g., allowing the user to create different or named views for different sets of modules in different places. My intuition is saying "keep it simple" and that we should allow one symlink home (and thus one view) for the user to control, otherwise the functionality will just be too confusing. Would a cluster really have use for managing more than one view?
haha yes I think this is what I just mentioned above, albeit with a different name! And I called the root symlink_home instead. I think instead of the nested structure my preference is to flatten things out, when possible, but I could be swayed to have a nested one.
So I'm not totally following here - right no we have the concept of namespaces, but that is to interact with modules for shpc. E.g., you can load a namespace ghcr.io/autamus and then just refer to anything under it with shpc, but that doesn't extend to the module software (e.g., you still load ghcr.io/autamus/. I think my concern here with adding custom namespaces is that if it's different for each module, we don't have a consistent way to find each one again on, for example, an uninstall (which i forgot to implement here).
omg yes I totally forgot to implement this!! I'll give it a go tomorrow. As long as the symlink tree home or root is provided, we should be able to predict the roots and do the uninstall akin to how we do in the regular module tree. If the user creates a tree and then changes it, they are on their own!
Can you give some examples here? I was noticing that the .version file didn't seem to be removing the "module" at the end, and I'm wondering if I created it incorrectly or there is something else we can do to better handle this default version. Okay I'm going back to bed! I will do another round of work on this tomorrow evening after some more sleep and a workday 😆 There is also good discussion starting in #501 about updates, and I think we should spec out an update command group (and possibly deprecate the check functionality). E.g., it would be nice for the user to not to have to rely on pulling updates from github, which only happen once a month, and just to be able to run:
but we need to distinguish between:
Something to think about - how would y'all like the above interactions to look? Should there be different flags / options or a more interactive workflow? Okay back to bed. Hooray Thursday coming up almost Friday!! |
also adding support for custom config on the fly with -c and for removing symlinks on uninstall Signed-off-by: vsoch <[email protected]>
okay updated with:
|
Signed-off-by: vsoch <[email protected]>
Agree on your feedback, in particular the "keep it simple" statement. Two suggestions for consistency:
I think a "flat" settings structure, with And just to make sure I have understood it right, the symlink_tree itself has a structure based solely on container and , correct? On my mention of namespaces, I was referring to this comment on yours in #456:
But I don't think it's a priority right now. On the Not diving in the |
Signed-off-by: vsoch <[email protected]>
Signed-off-by: vsoch <[email protected]>
okay!
It would just be based on the container name and version, so this means the user can technically install different modules (with the same base container name) to the same top level directory given they have the same versions. At first I was thinking we'd make them symlink the entire module directory (with version subfolders) but then I realized it was much easier to maintain one level lower, at the level of the version. So when we are parsing an install what we symlink is the
So this is sort of the default now? Everything is "proceed at your own risk" and if a duplicate name/version is found the user ca choose to proceed or not. The other options in that list were based on implementing it one level up. I think this is simpler and probably more flexible to mix things up.
You actually have a good point - I think the .version file is generated one level above that, so we would need to clean it up. I'm not doing that at the moment in this PR but I can take a look to add it.
TOTALLY agree lets focus on the symlink tree and the added config variable here - nothing else exists until we got this underway and merged! |
makes sense! so, on |
Signed-off-by: vsoch <[email protected]>
…is true Signed-off-by: vsoch <[email protected]>
Yes exactly! So I do that same check on creation of the symlink directory and create the .version file if needed: # If we don't have a version file in root, create it
if self.module_extension != "tcl" and self.settings.default_version == True:
version_file = os.path.join(os.path.dirname(symlink_path), ".version")
if not os.path.exists(version_file):
Path(version_file).touch() and just this evening I added the same check on cleanup - look to see if the parent of the version folder (which is the one symlinked) is empty or JUST has the version file, and if so, delete it (it's empty) # If the parent of the symlink only has zero files OR one file .version, cleanup
files = os.listdir(parent_dir)
if len(files) == 0 or (len(files) == 1 and files[0] == ".version"):
shutil.rmtree(parent_dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi. Thanks a lot for writing all of this.
The first bug I'm facing is that the global setting symlink_tree
doesn't seem to work. The only way I can get the symlinks is by adding the --symlink-tree
on the command-line. Could it be because it's spelt with an hyphen and an underscore throughout the codebase ?
$ git grep symlink_tree
docs/getting_started/user-guide.rst: * - symlink_tree
shpc/main/modules/__init__.py: symlink = self.settings.symlink_tree is True or symlink
shpc/main/schemas.py: "symlink_tree": {"type": "boolean"},
shpc/settings.yml:symlink_tree: true
$ git grep symlink-tree
.github/workflows/test.yml: shpc install python:3.9.5-alpine --symlink-tree
docs/getting_started/user-guide.rst: - If set, where you want to install a simplified module tree to using ``--symlink-tree``
docs/getting_started/user-guide.rst: - If set to true, ALWAYS generate a symlink tree given that a symlink base is defined regardless of ``--symlink-tree`` f
docs/getting_started/user-guide.rst: $ shpc install ghcr.io/autamus/clingo --symlink-tree
docs/getting_started/user-guide.rst: $ shpc install ghcr.io/autamus/samtools --symlink-tree
docs/getting_started/user-guide.rst: shpc install $module --symlink-tree
shpc/client/__init__.py: "--symlink-tree",
we can have a default of $root_dir/symlinks instead Signed-off-by: vsoch <[email protected]>
…e CLI argument, not the .yml setting (#509)
Signed-off-by: vsoch <[email protected]>
* 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 Co-authored-by: Vanessasaurus <[email protected]>
So, if I understand correctly, @muffato , your changes impact the dir/file naming for the symlink tree, from This is a great point, which however I think requires some extra care. In fact, this change is not only cosmetic, but it also has functional implications, that are not documented in the Lmod/EnvModules but still existing. The first syntax (one We had similar conversations in this Github project in the past, and @vsoch ended up implementing a setting I hope this makes sense, if not I am happy to chat further. |
That doesn't seem to be the case on our installation of modules here (
It's able to load a module from a prefix, as long as the prefix is unique (it wouldn't be able to load What version do you have installed ? Regardless, I understand how useful preventing a default version to be loaded, though, and I'd be happy to keep that possibility. We use this trick here in a different module tree, that I'd be looking into putting in our
|
cool! - didn't have the chance to test, but feel free to go ahead for what I am concerned, sounds like a great step forward |
Hi @vsoch, @muffato,
Basically, it causes an error if the module name to be loaded does not match one of So the proposed new implementation of
My understanding is that in this current PR, I am not able to implement it right now, but I think it is quite a small edit to do, the main thing to keep in mind is passing What do you think? |
I'll defer to @muffato, who (I think) has a setup to test these out! Note we are working on a double nested PR - there is a PR to this PR from @muffato that I've PR'd to, muffato#1. @muffato if you have preference for an implementation let me know and I'll update there, and we can eventually work our way back here! And indeed we have a conflict since a merge this morning - don't worry about this for now, since I've worked on both I'll be happy to do the resolution when the time comes |
TODO: when this set of PRs is merged remember to ping @dtrudg about giving a talk for SHPC at a singularity community call. |
Finally getting back to @marcodelapierre 's comment. |
Hi @muffato, apologies I wast not clear in my message. The solution that would NOT work with older versions is that which uses the new Lmod function To make my solution above work with older versions, I have not used such new function directly. Instead, I have explicitly used the implementation of such function. And, just to be clear, It's this one, copy-pasted from above:
I don't know exactly when these Does it make more sense Matthieu? |
* 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
Hiya. 2 comments: singularity-hpc/shpc/main/modules/__init__.py Line 191 in 18a76ee
singularity-hpc/shpc/main/modules/__init__.py Line 210 in 18a76ee
module_sys and null .Easy to fix but I've also given a try to @marcodelapierre's trick and I can confirm it works. It would remove the need for checking the value of default_version in these two lines, and do it in the template instead.
Just need to make a couple more tests before committing and pushing. One thing I want to raise first is that it may look confusing for users:
(only setting:
How does that sound ? |
Sounds good! Just to give you a heads up on this PR - the rebase is gnarly so I'll need to do it locally, probably after dinner tonight! |
it looks like we need to expose the force argument back for the symlink install? Signed-off-by: vsoch <[email protected]>
Signed-off-by: vsoch <[email protected]>
Signed-off-by: vsoch <[email protected]>
Signed-off-by: vsoch <[email protected]>
Signed-off-by: vsoch <[email protected]>
Huzzah tests pass! Ok I'm exhausted tonight and need to work on some other things, but let's all give this another look (if not the weekend next week) and have more discussion. Thanks for all the hard work y'all this is looking good! |
Cool 🚀 , thanks ! I'll push my last updates to my branch later today, and make a PR against your branch ? (not everything could fit within suggestions on this PR). |
Yes that would be great! |
@muffato you're right, that spurious "D" for Default is inconsistent with the failing module without Default. |
@marcodelapierre . I still pushed the change to #539 . Maybe the error message should have something like "Default modules are disabled by your systems administrator. Please specify a version" to make it clearer it's not Lmod's fault ? |
Good point @muffato, I like the alternative message you propose! |
Done |
* 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) Signed-off-by: vsoch <[email protected]> Co-authored-by: vsoch <[email protected]>
See extension of this work into "shpc views" here: #545 |
* 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 <[email protected]> Co-authored-by: vsoch <[email protected]> Co-authored-by: Matthieu Muffato <[email protected]> Co-authored-by: Matthieu Muffato <[email protected]>
Preceded by #545 |
This is a first shot at discussion from #456 (comment).
See https://github.com/singularityhub/singularity-hpc/pull/502/files#diff-2fc3b9f958ccce7c952b2460ec359be8c54adb468a2e6908328c8537066d1419R365-R415 for usage.
Signed-off-by: vsoch [email protected]