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

add support for specifying dependencies required to obtain source files via source_deps easyconfig parameter #4766

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

PetrKralCZ
Copy link
Contributor

(created using eb --new-pr)

@PetrKralCZ PetrKralCZ marked this pull request as draft February 18, 2025 11:12
@PetrKralCZ
Copy link
Contributor Author

When testing on Geneformer with git-lfs in source_deps:

ERROR: Traceback (most recent call last):
    File "/user/gent/453/vsc45304/easybuild/easybuild-framework/easybuild/main.py", line 137, in build_and_install_software
    (ec_res['success'], app_log, err) = build_and_install_one(ec, init_env)
    File "/user/gent/453/vsc45304/easybuild/easybuild-framework/easybuild/framework/easyblock.py", line 4295, in build_and_install_one
    result = app.run_all_steps(run_test_cases=run_test_cases)
    File "/user/gent/453/vsc45304/easybuild/easybuild-framework/easybuild/framework/easyblock.py", line 4174, in run_all_steps
    self.run_step(step_name, step_methods)
    File "/user/gent/453/vsc45304/easybuild/easybuild-framework/easybuild/framework/easyblock.py", line 4009, in run_step
    step_method(self)()
    File "/user/gent/453/vsc45304/easybuild/easybuild-framework/easybuild/framework/easyblock.py", line 2387, in fetch_step
    self.modules_tool.load(source_deps_mod_names)
    File "/user/gent/453/vsc45304/easybuild/easybuild-framework/easybuild/tools/modules.py", line 696, in load
    self.run_module('load', mod)
    File "/user/gent/453/vsc45304/easybuild/easybuild-framework/easybuild/tools/modules.py", line 827, in run_module
    full_cmd = ' '.join(cmd_list)
TypeError: sequence item 3: expected str instance, NoneType found

@boegel boegel added this to the 4.x milestone Feb 18, 2025
@boegel
Copy link
Member

boegel commented Feb 18, 2025

@PetrKralCZ How are you specifying source_deps exactly in your Geneformer easyconfig?

@PetrKralCZ
Copy link
Contributor Author

@PetrKralCZ How are you specifying source_deps exactly in your Geneformer easyconfig?

source_deps = [
    ('git-lfs', '3.5.1', '', SYSTEM),
]

@boegel
Copy link
Member

boegel commented Feb 18, 2025

We should make sure that entries in source_deps are also reported in the output produced by eb example.eb -Dr...

@boegel
Copy link
Member

boegel commented Feb 18, 2025

Fails with --fetch:

  File "/tmp/vsc40023/easybuild-framework/easybuild/framework/easyblock.py", line 2387, in fetch_step
    self.modules_tool.load(source_deps_mod_names)
  File "/tmp/vsc40023/easybuild-framework/easybuild/tools/modules.py", line 696, in load
    self.run_module('load', mod)
  File "/tmp/vsc40023/easybuild-framework/easybuild/tools/modules.py", line 814, in run_module
    self.log.debug('Current MODULEPATH: %s' % os.environ.get('MODULEPATH', '<unset>'))
AttributeError: 'NoModulesTool' object has no attribute 'log'

In that case there's no real ModulesTool instance in place, only a fake one (that's done in tools/options.py, via self.options.modules_tool = None).
That's... annyoing :)

@@ -766,6 +766,14 @@ def remove_false_versions(deps):
builddeps = [self._parse_dependency(dep, build_only=True) for dep in builddeps]
self['builddependencies'] = remove_false_versions(builddeps)

sourcedeps = self['source_deps']
if sourcedeps and all(isinstance(x, (list, tuple)) for b in sourcedeps for x in b):
self.iterate_options.append('source_deps')
Copy link
Member

Choose a reason for hiding this comment

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

source_deps should not be an iterative parameter (see also https://docs.easybuild.io/writing-easyconfig-files/#configure_build_install_command_options_iterate)

Suggested change
self.iterate_options.append('source_deps')

@@ -766,6 +766,14 @@ def remove_false_versions(deps):
builddeps = [self._parse_dependency(dep, build_only=True) for dep in builddeps]
self['builddependencies'] = remove_false_versions(builddeps)

sourcedeps = self['source_deps']
Copy link
Member

Choose a reason for hiding this comment

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

@PetrKralCZ Let's avoid confusion and use source_deps consistently (more changes needed below):

Suggested change
sourcedeps = self['source_deps']
source_deps = self['source_deps']

if source_deps:
pre_fetch_env = copy.deepcopy(os.environ)
source_deps_mod_names = [d['short_mod_name'] for d in source_deps]
self.modules_tool.load(source_deps_mod_names)
Copy link
Member

Choose a reason for hiding this comment

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

@PetrKralCZ Before trying to load modules, we need to take into account that self.modules_tool may be a NoModulesTool instance (that will be the case when eb --fetch is used).

If so, then we need to re-initialize self.modules_tool to a proper ModulesTool instance (via the modules_tool function available in tools/config.py).

This will need changes in options.py too though, because there we're now overwriting self.options.modules_tool with None, we also need to keep track of the original value so we can then pass it to the modules_tool function (which will also need to be adjusted)...

# load modules for source dependencies (if any)
if source_deps:
pre_fetch_env = copy.deepcopy(os.environ)
source_deps_mod_names = [d['short_mod_name'] for d in source_deps]
Copy link
Member

Choose a reason for hiding this comment

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

@PetrKralCZ short_mod_name field is still None at this point, which causes the crash in run_module, so we're overlooking something when parsing dependencies I think...

@boegel boegel changed the title Implement source_deps add support for specifying dependencies required to obtain source files via source_deps easyconfig parameter Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants