Skip to content

Prefer $EBPYTHONPREFIXES over $PYTHONPATH #4496

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.

Sign up for GitHub

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

Closed
Micket wants to merge 17 commits into from
Closed

Prefer $EBPYTHONPREFIXES over $PYTHONPATH #4496

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
0ec2df5
Make new safe module abstraction for update_paths
Micket Apr 3, 2024
74a3cb3
Replace PYTHONPATHs with EBPYTHONPREFIX when possible
Micket Apr 3, 2024
03e304e
Add option prefer-ebpythonprefix-over-pythonpath
Micket Apr 4, 2024
b07ea3f
Add warning for duplicated PYTHONPATH's that should not occur
Micket Apr 9, 2024
8c66e1a
Add tests for PYTHONPATH to EBPYTHONPREFIX rewrite
Micket Apr 10, 2024
ccd3d7e
Move check for faulty PYTHONPATHs to always run it
Micket Apr 11, 2024
f76f320
Move conversion of paths as str to lists earlier removing duplicated …
Micket Apr 11, 2024
8b5389f
Fix name EBPYTHONPREFIX to EBPYTHONPREFIXES which is used in sitecust…
Micket Apr 11, 2024
cb6adb0
Move _filter_paths check to _update_paths
Micket Apr 12, 2024
4a11f2f
Simplify logic for _filter_paths
Micket Apr 14, 2024
c471e03
Rename flag to replace_pythonpath
Micket Apr 21, 2024
2676e4e
Add assertion for input argument to _filter_paths.
Micket Apr 21, 2024
f4fbe76
Improve typing hints for list of str
Micket Apr 21, 2024
92f0e62
Also use List from typing
Micket Apr 21, 2024
e91186e
Improve typing hints for list of str
Micket Apr 21, 2024
236c67f
Improve typing hints for list of str
Micket Apr 21, 2024
ca304fb
Update easybuild/tools/module_generator.py
Micket Apr 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions easybuild/tools/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,7 @@ def mk_full_default_path(name, prefix=DEFAULT_PREFIX):
'map_toolchains',
'modules_tool_version_check',
'mpi_tests',
'prefer_ebpythonprefix_over_pythonpath',
'pre_create_installdir',
'show_progress_bar',
'trace',
Expand Down
79 changes: 44 additions & 35 deletions easybuild/tools/module_generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,55 +213,68 @@ def _filter_paths(self, key, paths):
return paths

added_paths = self.added_paths_per_key.setdefault(key, set())
# paths can be a string
if isinstance(paths, str):
if paths in added_paths:
filtered_paths = None
else:
added_paths.add(paths)
filtered_paths = paths
else:
# Coerce any iterable/generator into a list
if not isinstance(paths, list):
paths = list(paths)
filtered_paths = [x for x in paths if x not in added_paths and not added_paths.add(x)]
filtered_paths = [x for x in paths if x not in added_paths and not added_paths.add(x)]
if filtered_paths != paths:
removed_paths = paths if filtered_paths is None else [x for x in paths if x not in filtered_paths]
print_warning("Suppressed adding the following path(s) to $%s of the module as they were already added: %s",
Comment thread
Micket marked this conversation as resolved.
key, removed_paths,
key, ', '.join(removed_paths),
log=self.log)
if not filtered_paths:
filtered_paths = None
return filtered_paths

def append_paths(self, key, paths, allow_abs=False, expand_relpaths=True):
"""
Generate append-path statements for the given list of paths.

:param key: environment variable to append paths to
:param paths: list of paths to append
:param paths: single or list of paths to append
:param allow_abs: allow providing of absolute paths
:param expand_relpaths: expand relative paths into absolute paths (by prefixing install dir)
"""
paths = self._filter_paths(key, paths)
if paths is None:
return ''
return self.update_paths(key, paths, prepend=False, allow_abs=allow_abs, expand_relpaths=expand_relpaths)

def prepend_paths(self, key, paths, allow_abs=False, expand_relpaths=True):
"""
Generate prepend-path statements for the given list of paths.

:param key: environment variable to append paths to
:param paths: list of paths to append
:param paths: single or list of paths to append
:param allow_abs: allow providing of absolute paths
:param expand_relpaths: expand relative paths into absolute paths (by prefixing install dir)
"""
paths = self._filter_paths(key, paths)
if paths is None:
return ''
return self.update_paths(key, paths, prepend=True, allow_abs=allow_abs, expand_relpaths=expand_relpaths)

def update_paths(self, key, paths, prepend=True, allow_abs=False, expand_relpaths=True):
"""
Generate append/prepend-path statements for the given list of paths.

:param key: environment variable to append paths to
:param paths: single or list of paths to append
:param allow_abs: allow providing of absolute paths
:param expand_relpaths: expand relative paths into absolute paths (by prefixing install dir)
"""
if isinstance(paths, str):
self.log.debug("Wrapping %s into a list before using it for %s", paths, key)
paths = [paths]
elif not isinstance(paths, list):
paths = list(paths) # coerce any iterator into a list

if key == 'PYTHONPATH':
python_paths = [path for path in paths if re.match(r'lib/python\d+\.\d+/site-packages', path)]

@Flamefire Flamefire Apr 12, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Checking the logic in _filter_paths I see an issue here: You iterate paths multiple times: Here and at https://github.com/easybuilders/easybuild-framework/pull/4496/files#diff-4af72e9777d353325a29df400ab4229e14defd653e413db51786dc2172d3baefR275 but inside _update_paths you call _filter_paths which "coerces a generator into a list" if it isn't a list yet.

This means it this point paths might be a generator which can only be iterated once as otherwise the code in _filter_paths wasn't required.

Similar to my previous reasoning about breaking change in favor of cleaner code: I would simply disallow passing anything but a a string or a list to update_paths and assert that it is a list in _filter_paths instead of trying to convert a potentially exhausted generator to a list here or there.

Quick example:

paths=(i for i in range(10))
[i for i in paths if i==2] # [2]
[i for i in paths if i==2] # []

Comment thread
Micket marked this conversation as resolved.
Outdated
if len(python_paths) > 1:
raise EasyBuildError('Found multiple paths for PYTHONPATH: ' + ', '.join(python_paths))

# Special condition for PYTHONPATHs that match the standard pattern,
# replace with EBPYTHONPREFIX which is added to python sys path at runtime
if python_paths and build_option('prefer_ebpythonprefix_over_pythonpath'):
python_path = python_paths[0]
self.log.info("Replaced PYTHONPATH %s with EBPYTHONPREFIXES", python_path)
ret = self._update_paths('EBPYTHONPREFIXES', [''], prepend, allow_abs, expand_relpaths)
paths = [path for path in paths if path != python_path]
if paths:
ret += self._update_paths(key, paths, prepend, allow_abs, expand_relpaths)
return ret
return self._update_paths(key, paths, prepend, allow_abs, expand_relpaths)

def _modulerc_check_module_version(self, module_version):
"""
Check value type & contents of specified module-version spec.
Expand Down Expand Up @@ -551,7 +564,7 @@ def unload_module(self, mod_name):
"""
raise NotImplementedError

def update_paths(self, key, paths, prepend=True, allow_abs=False, expand_relpaths=True):
def _update_paths(self, key, paths, prepend=True, allow_abs=False, expand_relpaths=True):
"""
Generate prepend-path or append-path statements for the given list of paths.

Expand Down Expand Up @@ -955,16 +968,18 @@ def msg_on_unload(self, msg):
print_cmd = "puts stderr %s" % quote_str(msg, tcl=True)
return '\n'.join(['', self.conditional_statement("module-info mode unload", print_cmd, indent=False)])

def update_paths(self, key, paths, prepend=True, allow_abs=False, expand_relpaths=True):
def _update_paths(self, key, paths, prepend=True, allow_abs=False, expand_relpaths=True):
"""
Generate prepend-path or append-path statements for the given list of paths.

:param key: environment variable to prepend/append paths to
:param paths: list of paths to prepend
:param paths: list of paths to prepend/append
Comment thread
Micket marked this conversation as resolved.
Outdated
:param prepend: whether to prepend (True) or append (False) paths
:param allow_abs: allow providing of absolute paths
:param expand_relpaths: expand relative paths into absolute paths (by prefixing install dir)
"""
paths = self._filter_paths(key, paths)

if prepend:
update_type = 'prepend'
else:
Expand All @@ -974,10 +989,6 @@ def update_paths(self, key, paths, prepend=True, allow_abs=False, expand_relpath
self.log.info("Not including statement to %s environment variable $%s, as specified", update_type, key)
return ''

if isinstance(paths, str):
self.log.debug("Wrapping %s into a list before using it to %s path %s", paths, update_type, key)
paths = [paths]

abspaths = []
for path in paths:
if os.path.isabs(path) and not allow_abs:
Expand Down Expand Up @@ -1426,7 +1437,7 @@ def modulerc(self, module_version=None, filepath=None, modulerc_txt=None):
return super(ModuleGeneratorLua, self).modulerc(module_version=module_version, filepath=filepath,
modulerc_txt=modulerc_txt)

def update_paths(self, key, paths, prepend=True, allow_abs=False, expand_relpaths=True):
def _update_paths(self, key, paths, prepend=True, allow_abs=False, expand_relpaths=True):
"""
Generate prepend_path or append_path statements for the given list of paths

Expand All @@ -1436,6 +1447,8 @@ def update_paths(self, key, paths, prepend=True, allow_abs=False, expand_relpath
:param allow_abs: allow providing of absolute paths
:param expand_relpaths: expand relative paths into absolute paths (by prefixing install dir)
"""
paths = self._filter_paths(key, paths)

if prepend:
update_type = 'prepend'
else:
Expand All @@ -1445,10 +1458,6 @@ def update_paths(self, key, paths, prepend=True, allow_abs=False, expand_relpath
self.log.info("Not including statement to %s environment variable $%s, as specified", update_type, key)
return ''

if isinstance(paths, str):
self.log.debug("Wrapping %s into a list before using it to %s path %s", update_type, paths, key)
paths = [paths]

abspaths = []
for path in paths:
if os.path.isabs(path):
Expand Down
2 changes: 2 additions & 0 deletions easybuild/tools/options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,8 @@ def override_options(self):
'int', 'store', None),
'parallel-extensions-install': ("Install list of extensions in parallel (if supported)",
None, 'store_true', False),
'prefer-ebpythonprefix-over-pythonpath': ("Replaces PYTHONPATH with EBPYTHONPREFIX when possible",

@boegel boegel Apr 19, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

--prefer-ebpythonprefixes is specific enough, I think

@Micket Micket Apr 21, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We bounced around a bunch of different names, so i'm currently thinking maybe --replace-pythonpath is good enough. The EBPYTHONPREFIXES name is kind of an internal implementation detail in easybuild which wouldn't mean much to any user. The comment and documentation could go into more depth to explain it and how it connects to the site customize script our python installs have.

The fact that it doesn't always replace pythonpath, exceptions really are very rare, and this wouldn't be the first option that does what is claims only when it is supported.

None, 'store_true', True),
'pre-create-installdir': ("Create installation directory before submitting build jobs",
None, 'store_true', True),
'pretend': (("Does the build/installation in a test directory located in $HOME/easybuildinstall"),
Expand Down
7 changes: 7 additions & 0 deletions test/framework/module_generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -694,6 +694,9 @@ def append_paths(*args, **kwargs):
res = append_paths('key', ['1234@example.com'], expand_relpaths=False)
self.assertEqual("append-path\tkey\t\t1234@example.com\n", res)

expected = "append-path\tEBPYTHONPREFIXES\t\t$root\nappend-path\tPYTHONPATH\t\t$root/foo\n"
res = append_paths('PYTHONPATH', ['lib/python3.12/site-packages', 'foo'])
self.assertEqual(expected, res)
else:
expected = ''.join([
'append_path("key", pathJoin(root, "path1"))\n',
Expand All @@ -714,6 +717,10 @@ def append_paths(*args, **kwargs):
res = append_paths('key', ['1234@example.com'], expand_relpaths=False)
self.assertEqual('append_path("key", "1234@example.com")\n', res)

expected = 'append_path("EBPYTHONPREFIXES", root)\nappend_path("PYTHONPATH", pathJoin(root, "foo"))\n'
res = append_paths('PYTHONPATH', ['lib/python3.12/site-packages', 'foo'])
self.assertEqual(expected, res)

self.assertErrorRegex(EasyBuildError, "Absolute path %s/foo passed to update_paths "
"which only expects relative paths." % self.modgen.app.installdir,
append_paths, "key2", ["bar", "%s/foo" % self.modgen.app.installdir])
Expand Down