Introducing version_naming property that makes modulefile as version … #679

Amjadhpc · 2025-01-01T09:32:42Z

This is an attempt to fix issue number 26 raised by me.
I have not tested all settings

shpc/main/modules/base.py

vsoch · 2025-01-02T00:36:56Z

Looks like there are issues with the updated runner - let me see what I can do in another PR.

…number

vsoch · 2025-01-02T01:55:40Z

@Amjadhpc could you please add a test that enables the new setting and ensures it produces the right output? And @muffato I'm interested in your feedback here since you are correct views could be an alternative solution.

vsoch · 2025-01-05T19:21:33Z

I think you probably want to try:

client.settings.set("version_naming", True)
assert client.settings.get("version_naming") == True

vsoch · 2025-01-05T21:17:35Z

shpc/tests/test_version_naming.py

@@ -69,7 +69,7 @@ def test_disabled_version_naming_install(
    client = init_client(str(tmp_path), module_sys, container_tech)

    client.settings.set("version_naming", False)
-    assert client.settings.set("version_naming", False) is False
+    assert client.settings.set("version_naming") == False


You need to first set it, and then use get to check the setting is what you set it to.

shpc/tests/test_version_naming.py

vsoch · 2025-01-08T03:17:00Z

Nicely done! You'll want to (as a final step) bump the version in shpc/version.py and add a note to the CHANGELOG.md. @marcodelapierre and @muffato do you have any comments for the review?

vsoch · 2025-01-08T03:18:24Z

shpc/main/modules/base.py

@@ -470,7 +473,14 @@ def install(

        # Get the template based on the module and container type
        template = self.template.load(self.templatefile)
-        module_path = os.path.join(module.module_dir, self.modulefile)
+        if self.settings.version_naming:
+            self._modulefile = name.split(":")[1]


What is self._modulefile used for? I only see it in this small set of statements. It might make sense to just use name.split(":")[1] in the one place it is needed and not save a private class variable.

marcodelapierre · 2025-01-08T04:56:32Z

Hi, it is always great to see contributions coming from users!

As per @muffato 's comment in #678 ,
I would like to know whether module views do not fit your usage scenario and in which aspects.
In fact, the idea of views was to have a solid default structure for the default module tree, and then offer ways to customise the module tree in a user defined view. As @muffato mentions in the original issue, customising the naming of modules is one of the views' features.

I think it would be good to know whether Views can work in your user case or not, to try and not duplicate functionalities if not needed. Either way, discussing these aspects with users of the software has a great deal of added value, in making sure the software is useful to the community, both in terms of functionality and documentation on how to use them.

My 2 cents :)

vsoch · 2025-01-08T05:04:22Z

I agree it's not good to duplicate functionality, but in that views adds complexity (and this change makes it possible to generate the default with versions) I'm wondering if this isn't a good addition regardless.

marcodelapierre · 2025-01-09T02:13:09Z

shpc/settings.yml

@@ -68,6 +68,9 @@ singularity_shell: /bin/sh
 podman_shell: /bin/sh
 docker_shell: /bin/sh

+#Experimental version naming tag so that modulefile is changed to version number . Default value is false. Issue number 26 


I suggest having this right after default_version option, line 39, as they are related

marcodelapierre · 2025-01-09T02:15:22Z

Good point @vsoch :)

I will not have the chance to test this PR.

One comment I have is: is the PR been designed and tested while taking into account the implementation and options of the default_version setting? I bet there may be some specific requirements/adjustments coming out of it.

Amjadhpc · 2025-01-09T09:28:55Z

Nicely done! You'll want to (as a final step) bump the version in shpc/version.py and add a note to the CHANGELOG.md. @marcodelapierre and @muffato do you have any comments for the review?

The version in shpc/version.py is
version = "0.1.29"

You want me to bump it to "0.1.30"?
But there is no Release for 0.1.29, the last release was 0.1.28?

vsoch · 2025-01-09T13:24:10Z

shpc/main/modules/base.py

@@ -470,7 +473,13 @@ def install(

        # Get the template based on the module and container type
        template = self.template.load(self.templatefile)
-        module_path = os.path.join(module.module_dir, self.modulefile)
+        if self.settings.version_naming:
+            module._module_dir = os.path.dirname(module.module_dir)


Can you please walk me through why we need a private class variable here?

Sorry, forgot to clean it up, its not needed and have been fixed in new commit

vsoch · 2025-01-11T17:27:40Z

@Amjadhpc you'll need to go up to 0.1.31 now - we had another PR merge in this morning.

Amjadhpc · 2025-01-11T20:44:27Z

Looks like it has reached docker pull rate limit

WARNING: Couldn't use cached digest for registry: GET https://index.docker.io/v2/library/python/manifests/sha256:7dd8962ad2a63403428d652a64d814a5002f1386379355edf5970e40557fe4e6: TOOMANYREQUESTS: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit
WARNING: Falling back to direct digest.
FATAL: docker://python@sha256:7dd8962ad2a63403428d652a64d814a5002f1386379355edf5970e40557fe4e6: GET https://index.docker.io/v2/library/python/manifests/sha256:7dd8962ad2a63403428d652a64d814a5002f1386379355edf5970e40557fe4e6: TOOMANYREQUESTS: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit

vsoch · 2025-01-11T20:48:04Z

@Amjadhpc it's an ephemeral error, OK to run again. I'm not going to put credentials in there just for these rare cases.

vsoch

Nice work @Amjadhpc !

Amjadhpc · 2025-01-11T20:57:10Z

Thanks for your help and guidance @vsoch

vsoch reviewed Jan 1, 2025

View reviewed changes

shpc/main/modules/base.py Outdated Show resolved Hide resolved

shpc/main/modules/base.py Outdated Show resolved Hide resolved

AmjadUEA added 2 commits January 1, 2025 18:47

Introducing version_naming property that makes modulefile as version …

53e085f

…number

Ran black to format the indenting

e1996fa

vsoch force-pushed the main branch from 0bdd808 to e1996fa Compare January 2, 2025 01:47

AmjadUEA added 2 commits January 5, 2025 17:56

Adding version_naming test

5d60af3

Trying to fix the test issue

2199f20

Fixing condition for Test

75240b5

vsoch reviewed Jan 5, 2025

View reviewed changes

Adding in get statement instead of set statement

4133ee7

vsoch reviewed Jan 6, 2025

View reviewed changes

shpc/tests/test_version_naming.py Show resolved Hide resolved

AmjadUEA added 2 commits January 6, 2025 11:54

Fixing line 46 replacing set with get

9483d2b

Adding in pytest.mark.parametize for disable install testing

5e0b994

vsoch reviewed Jan 8, 2025

View reviewed changes

marcodelapierre reviewed Jan 9, 2025

View reviewed changes

Adding in suggestions from PR review

6a47294

vsoch reviewed Jan 9, 2025

View reviewed changes

removing the private class variable forgot to cleanup

cc4be9b

Amjadhpc added 2 commits January 11, 2025 18:12

Merge branch 'main' into main

a929ac0

Bump to 0.1.31

52ae600

vsoch approved these changes Jan 11, 2025

View reviewed changes

vsoch merged commit 7af1150 into singularityhub:main Jan 11, 2025
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing version_naming property that makes modulefile as version … #679

Introducing version_naming property that makes modulefile as version … #679

Amjadhpc commented Jan 1, 2025

vsoch commented Jan 2, 2025

vsoch commented Jan 2, 2025

vsoch commented Jan 5, 2025

vsoch Jan 5, 2025

vsoch commented Jan 8, 2025

vsoch Jan 8, 2025

marcodelapierre commented Jan 8, 2025

vsoch commented Jan 8, 2025

marcodelapierre Jan 9, 2025

marcodelapierre commented Jan 9, 2025

Amjadhpc commented Jan 9, 2025

vsoch Jan 9, 2025

Amjadhpc Jan 11, 2025

vsoch commented Jan 11, 2025

Amjadhpc commented Jan 11, 2025

vsoch commented Jan 11, 2025

vsoch left a comment

Amjadhpc commented Jan 11, 2025

Introducing version_naming property that makes modulefile as version … #679

Introducing version_naming property that makes modulefile as version … #679

Conversation

Amjadhpc commented Jan 1, 2025

vsoch commented Jan 2, 2025

vsoch commented Jan 2, 2025

vsoch commented Jan 5, 2025

vsoch Jan 5, 2025

Choose a reason for hiding this comment

vsoch commented Jan 8, 2025

vsoch Jan 8, 2025

Choose a reason for hiding this comment

marcodelapierre commented Jan 8, 2025

vsoch commented Jan 8, 2025

marcodelapierre Jan 9, 2025

Choose a reason for hiding this comment

marcodelapierre commented Jan 9, 2025

Amjadhpc commented Jan 9, 2025

vsoch Jan 9, 2025

Choose a reason for hiding this comment

Amjadhpc Jan 11, 2025

Choose a reason for hiding this comment

vsoch commented Jan 11, 2025

Amjadhpc commented Jan 11, 2025

vsoch commented Jan 11, 2025

vsoch left a comment

Choose a reason for hiding this comment

Amjadhpc commented Jan 11, 2025