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

enable --module-extensions by default (+ resolve template values used in extension version) #4501

Merged
merged 6 commits into from
Aug 28, 2024

Conversation

Micket
Copy link
Contributor

@Micket Micket commented Apr 4, 2024

fixes #4396

@Micket Micket added change EasyBuild-5.0 EasyBuild 5.0 labels Apr 4, 2024
@Micket Micket added this to the 5.0 milestone Apr 4, 2024
@Micket
Copy link
Contributor Author

Micket commented Apr 5, 2024

I'm very confused, out of the type of tests i expected to see failing, the one that actually broke is test_exts_list, the one the test is supposed to already be using this feature to see the list of extensions?? And It fails because it is missing homepage field.

ERROR: test_exts_list (test.framework.easyconfig.EasyConfigTest)
  File "/tmp/runner/d4b8e7c4de51b3c7733badf96aac3b5c5e83313c/lib/python3.6/site-packages/test/framework/easyconfig.py", line 523, in test_exts_list
    modfile = os.path.join(eb.make_module_step(), 'PI', '3.14' + eb.module_generator.MODULE_FILE_EXTENSION)
  File "/tmp/runner/d4b8e7c4de51b3c7733badf96aac3b5c5e83313c/lib/python3.6/site-packages/easybuild/framework/easyblock.py", line 3778, in make_module_step
    txt += self.make_module_description()
  File "/tmp/runner/d4b8e7c4de51b3c7733badf96aac3b5c5e83313c/lib/python3.6/site-packages/easybuild/framework/easyblock.py", line 1376, in make_module_description
    return self.module_generator.get_description()
  File "/tmp/runner/d4b8e7c4de51b3c7733badf96aac3b5c5e83313c/lib/python3.6/site-packages/easybuild/tools/module_generator.py", line 1302, in get_description
    'homepage': self.app.cfg['homepage'],

I can only assume this test was never actually running properly then? Confusing.

@Micket
Copy link
Contributor Author

Micket commented Apr 6, 2024

oh, it's just the test that is super misleading, issue has nothing to do with homepage. It was just due to the partial template resolving code that get_description attempts for some reason.

I think it started from needlessly using template to put in the whatis section, and then that template added name, version, homepage and a bunch of other common things since they happened to be there. But this code shouldn't be doing this, it should leave templates alone. It certainly isn't necesssary to add the whatis section, and this change to just write the lines in order directly is shorter, simpler.

Old method needlessly involved template resolutions,
duplicating the template resolving code, but only partially
and would break if anything but the 4 most common templates were used.
@Micket
Copy link
Contributor Author

Micket commented Apr 8, 2024

I'm struggling with the test suite, because of the already existing bug that breaks --module-extensions=True.

  1. The test test_exts_list is implying that this
    ("ext-%(namelower)s", "%(version_major)s.0"),
    should be valid.
  2. The get_description (and some other parts of module_generator) randomly decided to resolve 3 template strings (name, version, installdir)
    'name': self.app.name,
    'version': self.app.version,
    'whatis_lines': '\n'.join(whatis_lines),
    'installdir': self.app.installdir,
    and nothing else.
  3. If one actually runs the suggested easyconfig, using version_major, it does not get expanded by make_extension_string
    exts_str = self.app.make_extension_string(name_version_sep='/', ext_sep=',')
    so, the unresolved version_major is there and EasyBuild will crash. Note: This is already present in easybuild, regardless of this PR.
  4. Given that %(name) and %(version) is supposed to be resolved to refer to the extension itself, it's a bit confusing to also use it as the name and version for an extension in my opinion. I understand the difference of if it's used for 2 first tuple fields of an extension, I still think it's confusing.

@boegel boegel added the EasyBuild-5.0-blocker Blocker for EasyBuild 5.0 label Jun 5, 2024
@Micket
Copy link
Contributor Author

Micket commented Aug 26, 2024

The bug here also is present in the generated EBEXTSLISTxxxx variables inside lua files, where you can find unresolved templates like %(name)s and %(version)s inside the string.

@boegel
Copy link
Member

boegel commented Aug 27, 2024

Concrete example of the bug in module for Transformers-4.39.3-gfbf-2023a.eb:

$ grep namelower /modules/all/Transformers/4.30.2-foss-2022b.lua
%(namelower)s-4.30.2, huggingface-hub-0.15.1, regex-2023.6.3,
whatis([==[Extensions: %(namelower)s-4.30.2, huggingface-hub-0.15.1, regex-2023.6.3, safetensors-0.3.1]==])
setenv("EBEXTSLISTTRANSFORMERS", "safetensors-0.3.1,regex-2023.6.3,huggingface-hub-0.15.1,%(namelower)s-4.30.2")

because Transformers itself is included as extension via:

exts_list = [
    ...
    ('%(namelower)s', version, {
        'checksums': ['2586e5ff4150f122716fc40f5530e92871befc051848fbe82600969c535b762d'],
    }),
]

@Micket
Copy link
Contributor Author

Micket commented Aug 27, 2024

@boegel Actually, looking at this now, i see this was partly fixed in:
#4392
so that module build predates this fix. However, it only does it for the first argument, not the version string, which is what is triggering this bug. So the fix is trivial.

@Micket Micket marked this pull request as ready for review August 27, 2024 23:59
@boegel boegel changed the title Make module-extensions true by default enable --module-extensions true by default (+ resolve template values used in extension version) Aug 28, 2024
@boegel boegel changed the title enable --module-extensions true by default (+ resolve template values used in extension version) enable --module-extensions by default (+ resolve template values used in extension version) Aug 28, 2024
boegel
boegel previously approved these changes Aug 28, 2024
Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

lgtm

@boegel boegel enabled auto-merge August 28, 2024 18:46
Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

lgtm

@boegel boegel merged commit 2e61f32 into easybuilders:5.0.x Aug 28, 2024
35 checks passed
@Micket Micket deleted the modextensionstrue branch August 29, 2024 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change EasyBuild-5.0-blocker Blocker for EasyBuild 5.0 EasyBuild-5.0 EasyBuild 5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants