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.

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

Jump to bottom

Prefer $EBPYTHONPREFIXES over $PYTHONPATH #4496

Draft
wants to merge 17 commits into
base: 5.0.x
Choose a base branch
from
Draft

Prefer $EBPYTHONPREFIXES over $PYTHONPATH #4496

Show file tree
Hide file tree
Changes from 9 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.
Jump to
Jump to file
Failed to load files.
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
76 changes: 46 additions & 30 deletions easybuild/tools/module_generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,22 +213,14 @@ 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)]
# Coerce any iterable/generator into a list
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit worried about the change here. Why changing _filter_paths and even removing the ability to pass a string (which is a breaking change leading to surprising behavior)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rest of the code (_update_paths) both converted the str case to a list. I simply moved this earlier, which also removes the need for _filter_paths to keep duplicate logic, since regardless it was always converted to a list immediately afterwards anyway.

You can pass a str to paths to append/prepend/update_paths.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I see that our usages in this file basically make this part dead code. However someone or something else might already use this code, so I'm not sure if we should be a bit more careful here. E.g. at least error out when a string is passed instead of an iterable of strings

On the other hand: Calling this method from outside would have been wrong (it is "private") and this PR is targetting 5.x so a breaking change for simpler logic is fine.

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)]
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",
Micket marked this conversation as resolved.
Show resolved Hide resolved
key, removed_paths,
key, ', '.join(removed_paths),
log=self.log)
if not filtered_paths:
filtered_paths = None
Expand All @@ -243,9 +235,6 @@ def append_paths(self, key, paths, allow_abs=False, expand_relpaths=True):
: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):
Expand All @@ -257,11 +246,38 @@ def prepend_paths(self, key, paths, allow_abs=False, expand_relpaths=True):
: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: 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]

if key == 'PYTHONPATH':
python_paths = [path for path in paths if re.match(r'lib/python\d+\.\d+/site-packages', path)]
Copy link
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] # []

Micket marked this conversation as resolved.
Show resolved Hide resolved
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 +567,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,7 +971,7 @@ 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.

Expand All @@ -965,6 +981,10 @@ 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 paths is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering about how the filtered paths could be None:
This is inside _filter_paths:

if not filtered_paths:
    filtered_paths = None

I would remove both checks, neither is required and we are actually checking stuff twice. Having _filter_paths always return a list, even if it is empty is cleaner interface-wise. I mean otherwise you would need to document it as "returns a non-empty list of paths to add or None" instead of "returns a list of paths to add".

If we really want to exit early here, then it is better to remove the above check in _filter_paths and change this to:

Suggested change
if paths is None:
if not paths:

That would at least check only once

return ''

if prepend:
update_type = 'prepend'
else:
Expand All @@ -974,10 +994,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 +1442,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 +1452,10 @@ 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 paths is None:
return ''

if prepend:
update_type = 'prepend'
else:
Expand All @@ -1445,10 +1465,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",
Copy link
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

Copy link
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