From 7726432d64a1f9232cebe079f58ac38bb458f5b0 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Thu, 29 Aug 2024 13:44:10 +0200 Subject: [PATCH 1/4] run `pip check` only once for PythonBundle --- easybuild/easyblocks/generic/pythonbundle.py | 31 ++- easybuild/easyblocks/generic/pythonpackage.py | 200 ++++++++++-------- 2 files changed, 139 insertions(+), 92 deletions(-) diff --git a/easybuild/easyblocks/generic/pythonbundle.py b/easybuild/easyblocks/generic/pythonbundle.py index d85579e356..0e2d2a1192 100644 --- a/easybuild/easyblocks/generic/pythonbundle.py +++ b/easybuild/easyblocks/generic/pythonbundle.py @@ -31,7 +31,8 @@ from easybuild.easyblocks.generic.bundle import Bundle from easybuild.easyblocks.generic.pythonpackage import EXTS_FILTER_PYTHON_PACKAGES -from easybuild.easyblocks.generic.pythonpackage import PythonPackage, get_pylibdirs, find_python_cmd_from_ec +from easybuild.easyblocks.generic.pythonpackage import (PythonPackage, + get_pylibdirs, find_python_cmd_from_ec, run_pip_check) from easybuild.tools.build_log import EasyBuildError from easybuild.tools.config import build_option, PYTHONPATH, EBPYTHONPREFIXES from easybuild.tools.modules import get_software_root @@ -162,6 +163,13 @@ def load_module(self, *args, **kwargs): def sanity_check_step(self, *args, **kwargs): """Custom sanity check for bundle of Python package.""" + if self.pylibdir is None: + # Python attributes not set up yet, happens e.g. with --sanity-check-only + # Load module first to get the right python command + self.fake_mod_data = self.sanity_check_load_module(extension=kwargs.get('extension', False), + extra_modules=kwargs.get('extra_modules', None)) + self.prepare_python() + # inject directory path that uses %(pyshortver)s template into default value for sanity_check_paths # this is relevant for installations of Python bundles for multiple Python versions (via multi_deps) # (we can not pass this via custom_paths, since then the %(pyshortver)s template value will not be resolved) @@ -172,3 +180,24 @@ def sanity_check_step(self, *args, **kwargs): } super(Bundle, self).sanity_check_step(*args, **kwargs) + + # After the super-call self.ext_instances is initialized, so we can check the extensions + sanity_pip_check = self.cfg['sanity_pip_check'] + unversioned_packages = set(self.cfg['unversioned_packages']) + has_sanity_pip_check_mismatch = False + all_unversioned_packages = unversioned_packages.copy() + for ext in self.ext_instances: + if isinstance(ext, PythonPackage): + if ext.cfg['sanity_pip_check'] != sanity_pip_check: + has_sanity_pip_check_mismatch = True + all_unversioned_packages.update(ext.cfg['unversioned_packages']) + + if has_sanity_pip_check_mismatch: + self.log.deprecated('For bundles of PythonPackage the sanity_pip_check option ' + 'in the main EasyConfig must be used', '5.0') + sanity_pip_check = True # Either the main set it or any extension enabled it + if all_unversioned_packages != unversioned_packages: + self.log.deprecated('For bundles of PythonPackage the unversioned_packages option ' + 'in the main EasyConfig must be used', '5.0') + if sanity_pip_check: + run_pip_check(self.log, self.python_cmd, all_unversioned_packages) diff --git a/easybuild/easyblocks/generic/pythonpackage.py b/easybuild/easyblocks/generic/pythonpackage.py index eda88a21ec..955c22ccca 100644 --- a/easybuild/easyblocks/generic/pythonpackage.py +++ b/easybuild/easyblocks/generic/pythonpackage.py @@ -389,6 +389,94 @@ def symlink_dist_site_packages(install_dir, pylibdirs): symlink(dist_pkgs, site_pkgs_path, use_abspath_source=False) +def det_installed_python_packages(log, names_only=True, python_cmd=None): + """Return list of Python packages that are installed + + When names_only is True then only the names are returned, else the full info from `pip list`. + Note that the names are reported by pip and might be different to the name that need to be used to import it + """ + # Check installed python packages but only check stdout, not stderr which might contain user facing warnings + cmd_list = [python_cmd, '-m', 'pip', 'list', '--isolated', '--disable-pip-version-check', + '--format', 'json'] + full_cmd = ' '.join(cmd_list) + log.info("Running command '%s'" % full_cmd) + proc = subprocess_popen_text(cmd_list, env=os.environ) + (stdout, stderr) = proc.communicate() + ec = proc.returncode + msg = "Command '%s' returned with %s: stdout: %s; stderr: %s" % (full_cmd, ec, stdout, stderr) + if ec: + log.info(msg) + raise EasyBuildError('Failed to determine installed python packages: %s', stderr) + + log.debug(msg) + pkgs = json.loads(stdout.strip()) + return [pkg['name'] for pkg in pkgs] if names_only else pkgs + + +def run_pip_check(log, python_cmd, unversioned_packages): + """Check installed Python packages using pip + + log - Logger + python_cmd - Python command + unversioned_packages - Python packages to exclude in the version existance check + """ + pip_check_command = "%s -m pip check" % python_cmd + pip_version = det_pip_version(python_cmd=python_cmd) + if not pip_version: + raise EasyBuildError("Failed to determine pip version!") + min_pip_version = LooseVersion('9.0.0') + if LooseVersion(pip_version) < min_pip_version: + raise EasyBuildError("pip >= %s is required for running '%s', found %s", + min_pip_version, pip_check_command, pip_version) + + pip_check_errors = [] + + res = run_shell_cmd(pip_check_command, fail_on_error=False) + if res.exit_code: + pip_check_errors.append('`%s` failed:\n%s' % (pip_check_command, res.output)) + else: + log.info('`%s` completed successfully' % pip_check_command) + + # Also check for a common issue where the package version shows up as 0.0.0 often caused + # by using setup.py as the installation method for a package which is released as a generic wheel + # named name-version-py2.py3-none-any.whl. `tox` creates those from version controlled source code + # so it will contain a version, but the raw tar.gz does not. + pkgs = det_installed_python_packages(log, names_only=False, python_cmd=python_cmd) + faulty_version = '0.0.0' + faulty_pkg_names = [pkg['name'] for pkg in pkgs if pkg['version'] == faulty_version] + + for unversioned_package in unversioned_packages: + try: + faulty_pkg_names.remove(unversioned_package) + log.debug('Excluding unversioned package %s from check', unversioned_package) + except ValueError: + try: + version = next(pkg['version'] for pkg in pkgs if pkg['name'] == unversioned_package) + except StopIteration: + msg = ('Package %s in unversioned_packages was not found in the installed packages. ' + 'Check that the name from `python -m pip list` is used which may be different ' + 'than the module name.' % unversioned_package) + else: + msg = ('Package %s in unversioned_packages has a version of %s which is valid. ' + 'Please remove it from unversioned_packages.' % (unversioned_package, version)) + pip_check_errors.append(msg) + + log.info('Found %s invalid packages out of %s packages', len(faulty_pkg_names), len(pkgs)) + if faulty_pkg_names: + msg = ( + "The following Python packages were likely not installed correctly because they show a " + "version of '%s':\n%s\n" + "This may be solved by using a *-none-any.whl file as the source instead. " + "See e.g. the SOURCE*_WHL templates.\n" + "Otherwise you could check if the package provides a version at all or if e.g. poetry is " + "required (check the source for a pyproject.toml and see PEP517 for details on that)." + ) % (faulty_version, '\n'.join(faulty_pkg_names)) + pip_check_errors.append(msg) + + if pip_check_errors: + raise EasyBuildError('\n'.join(pip_check_errors)) + + class PythonPackage(ExtensionEasyBlock): """Builds and installs a Python package, and provides a dedicated module file.""" @@ -594,25 +682,7 @@ def get_installed_python_packages(self, names_only=True, python_cmd=None): """ if python_cmd is None: python_cmd = self.python_cmd - # Check installed python packages but only check stdout, not stderr which might contain user facing warnings - cmd_list = [python_cmd, '-m', 'pip', 'list', '--isolated', '--disable-pip-version-check', - '--format', 'json'] - full_cmd = ' '.join(cmd_list) - self.log.info("Running command '%s'" % full_cmd) - proc = subprocess_popen_text(cmd_list, env=os.environ) - (stdout, stderr) = proc.communicate() - ec = proc.returncode - msg = "Command '%s' returned with %s: stdout: %s; stderr: %s" % (full_cmd, ec, stdout, stderr) - if ec: - self.log.info(msg) - raise EasyBuildError('Failed to determine installed python packages: %s', stderr) - - self.log.debug(msg) - pkgs = json.loads(stdout.strip()) - if names_only: - return [pkg['name'] for pkg in pkgs] - else: - return pkgs + return det_installed_python_packages(self.log, names_only, python_cmd) def using_pip_install(self): """ @@ -1053,78 +1123,26 @@ def sanity_check_step(self, *args, **kwargs): exts_filter = (orig_exts_filter[0].replace('python', self.python_cmd), orig_exts_filter[1]) kwargs.update({'exts_filter': exts_filter}) - if self.cfg.get('sanity_pip_check', True): - pip_version = det_pip_version(python_cmd=python_cmd) - - if pip_version: - pip_check_command = "%s -m pip check" % python_cmd - - if LooseVersion(pip_version) >= LooseVersion('9.0.0'): - - if not self.is_extension: - # for stand-alone Python package installations (not part of a bundle of extensions), - # the (fake or real) module file must be loaded at this point, - # otherwise the Python package being installed is not "in view", - # and we will overlook missing dependencies... - loaded_modules = [x['mod_name'] for x in self.modules_tool.list()] - if self.short_mod_name not in loaded_modules: - self.log.debug("Currently loaded modules: %s", loaded_modules) - raise EasyBuildError("%s module is not loaded, this should never happen...", - self.short_mod_name) - - pip_check_errors = [] - - res = run_shell_cmd(pip_check_command, fail_on_error=False) - pip_check_msg = res.output - if res.exit_code: - pip_check_errors.append('`%s` failed:\n%s' % (pip_check_command, pip_check_msg)) - else: - self.log.info('`%s` completed successfully' % pip_check_command) - - # Also check for a common issue where the package version shows up as 0.0.0 often caused - # by using setup.py as the installation method for a package which is released as a generic wheel - # named name-version-py2.py3-none-any.whl. `tox` creates those from version controlled source code - # so it will contain a version, but the raw tar.gz does not. - pkgs = self.get_installed_python_packages(names_only=False, python_cmd=python_cmd) - faulty_version = '0.0.0' - faulty_pkg_names = [pkg['name'] for pkg in pkgs if pkg['version'] == faulty_version] - - for unversioned_package in self.cfg.get('unversioned_packages', []): - try: - faulty_pkg_names.remove(unversioned_package) - self.log.debug('Excluding unversioned package %s from check', unversioned_package) - except ValueError: - try: - version = next(pkg['version'] for pkg in pkgs if pkg['name'] == unversioned_package) - except StopIteration: - msg = ('Package %s in unversioned_packages was not found in the installed packages. ' - 'Check that the name from `python -m pip list` is used which may be different ' - 'than the module name.' % unversioned_package) - else: - msg = ('Package %s in unversioned_packages has a version of %s which is valid. ' - 'Please remove it from unversioned_packages.' % (unversioned_package, version)) - pip_check_errors.append(msg) - - self.log.info('Found %s invalid packages out of %s packages', len(faulty_pkg_names), len(pkgs)) - if faulty_pkg_names: - msg = ( - "The following Python packages were likely not installed correctly because they show a " - "version of '%s':\n%s\n" - "This may be solved by using a *-none-any.whl file as the source instead. " - "See e.g. the SOURCE*_WHL templates.\n" - "Otherwise you could check if the package provides a version at all or if e.g. poetry is " - "required (check the source for a pyproject.toml and see PEP517 for details on that)." - ) % (faulty_version, '\n'.join(faulty_pkg_names)) - pip_check_errors.append(msg) - - if pip_check_errors: - raise EasyBuildError('\n'.join(pip_check_errors)) - else: - raise EasyBuildError("pip >= 9.0.0 is required for running '%s', found %s", - pip_check_command, - pip_version) - else: - raise EasyBuildError("Failed to determine pip version!") + sanity_pip_check = self.cfg.get('sanity_pip_check', True) + if self.is_extension: + sanity_pip_check_main = self.master.cfg.get('sanity_pip_check') + if sanity_pip_check_main is not None: + # If the main easyblock (e.g. PythonBundle) defines the variable + # we trust it does the pip check if requested and checks for mismatches + sanity_pip_check = False + + if sanity_pip_check: + if not self.is_extension: + # for stand-alone Python package installations (not part of a bundle of extensions), + # the (fake or real) module file must be loaded at this point, + # otherwise the Python package being installed is not "in view", + # and we will overlook missing dependencies... + loaded_modules = [x['mod_name'] for x in self.modules_tool.list()] + if self.short_mod_name not in loaded_modules: + self.log.debug("Currently loaded modules: %s", loaded_modules) + raise EasyBuildError("%s module is not loaded, this should never happen...", + self.short_mod_name) + run_pip_check(self.log, python_cmd, self.cfg.get('unversioned_packages', [])) # ExtensionEasyBlock handles loading modules correctly for multi_deps, so we clean up fake_mod_data # and let ExtensionEasyBlock do its job From 06ab7421ad3a96d8c7ca45b18e1949f1b515092c Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Fri, 30 Aug 2024 16:10:04 +0200 Subject: [PATCH 2/4] Fix failing sanity check due to (fake) module not being loaded --- easybuild/easyblocks/generic/pythonbundle.py | 20 ++++++++++++++----- easybuild/easyblocks/generic/pythonpackage.py | 4 ++-- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/easybuild/easyblocks/generic/pythonbundle.py b/easybuild/easyblocks/generic/pythonbundle.py index 0e2d2a1192..bf9a360abc 100644 --- a/easybuild/easyblocks/generic/pythonbundle.py +++ b/easybuild/easyblocks/generic/pythonbundle.py @@ -164,10 +164,12 @@ def sanity_check_step(self, *args, **kwargs): """Custom sanity check for bundle of Python package.""" if self.pylibdir is None: - # Python attributes not set up yet, happens e.g. with --sanity-check-only - # Load module first to get the right python command - self.fake_mod_data = self.sanity_check_load_module(extension=kwargs.get('extension', False), - extra_modules=kwargs.get('extra_modules', None)) + # Python attributes not set up yet, happens e.g. with --sanity-check-only, so do it now. + # This also ensures the exts_filter option for extensions is set correctly. + # Load module first to get the right python command. + if not self.sanity_check_module_loaded: + self.sanity_check_load_module(extension=kwargs.get('extension', False), + extra_modules=kwargs.get('extra_modules', None)) self.prepare_python() # inject directory path that uses %(pyshortver)s template into default value for sanity_check_paths @@ -181,9 +183,16 @@ def sanity_check_step(self, *args, **kwargs): super(Bundle, self).sanity_check_step(*args, **kwargs) - # After the super-call self.ext_instances is initialized, so we can check the extensions + def _sanity_check_step_extensions(self): + """Run the pip check for extensions if enabled""" + super(PythonBundle, self)._sanity_check_step_extensions() + sanity_pip_check = self.cfg['sanity_pip_check'] unversioned_packages = set(self.cfg['unversioned_packages']) + + # The options should be set in the main EC and cannot be different between extensions. + # For backwards compatibility and to avoid surprises enable the pip-check if it is enabled + # in the main EC or any extension and build the union of all unversioned_packages. has_sanity_pip_check_mismatch = False all_unversioned_packages = unversioned_packages.copy() for ext in self.ext_instances: @@ -199,5 +208,6 @@ def sanity_check_step(self, *args, **kwargs): if all_unversioned_packages != unversioned_packages: self.log.deprecated('For bundles of PythonPackage the unversioned_packages option ' 'in the main EasyConfig must be used', '5.0') + if sanity_pip_check: run_pip_check(self.log, self.python_cmd, all_unversioned_packages) diff --git a/easybuild/easyblocks/generic/pythonpackage.py b/easybuild/easyblocks/generic/pythonpackage.py index 955c22ccca..5bde33f324 100644 --- a/easybuild/easyblocks/generic/pythonpackage.py +++ b/easybuild/easyblocks/generic/pythonpackage.py @@ -1066,10 +1066,10 @@ def sanity_check_step(self, *args, **kwargs): # load module early ourselves rather than letting parent sanity_check_step method do so, # since custom actions taken below require that environment is set up properly already # (especially when using --sanity-check-only) - if hasattr(self, 'sanity_check_module_loaded') and not self.sanity_check_module_loaded: + if not self.sanity_check_module_loaded: extension = self.is_extension or kwargs.get('extension', False) extra_modules = kwargs.get('extra_modules', None) - self.fake_mod_data = self.sanity_check_load_module(extension=extension, extra_modules=extra_modules) + self.sanity_check_load_module(extension=extension, extra_modules=extra_modules) # don't add user site directory to sys.path (equivalent to python -s) # see https://www.python.org/dev/peps/pep-0370/; From 60480fac5f6aca40dbebfac11f4048e894d481c8 Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Wed, 9 Oct 2024 16:42:50 +0200 Subject: [PATCH 3/4] Error out when only `req_py_minver` is set --- easybuild/easyblocks/generic/pythonpackage.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/easybuild/easyblocks/generic/pythonpackage.py b/easybuild/easyblocks/generic/pythonpackage.py index 5bde33f324..b78868a2b4 100644 --- a/easybuild/easyblocks/generic/pythonpackage.py +++ b/easybuild/easyblocks/generic/pythonpackage.py @@ -194,8 +194,9 @@ def find_python_cmd(log, req_py_majver, req_py_minver, max_py_majver, max_py_min # if no Python version requirements are specified, # use major/minor version of Python being used in this EasyBuild session if req_py_majver is None: + if req_py_minver is not None: + raise EasyBuildError("'req_py_majver' must be specified when 'req_py_minver' is set!") req_py_majver = sys.version_info[0] - if req_py_minver is None: req_py_minver = sys.version_info[1] # if using system Python, go hunting for a 'python' command that satisfies the requirements python = pick_python_cmd(req_maj_ver=req_py_majver, req_min_ver=req_py_minver, From a3c7b5547494bac0d0cbd1606eac155e13579c3e Mon Sep 17 00:00:00 2001 From: Alexander Grund Date: Wed, 9 Oct 2024 18:07:35 +0200 Subject: [PATCH 4/4] Fix handling of maximum python version in `pick_python_cmd` When the minor version is unset, only the major version must be checked. Otherwise patch versions etc. must be ignored. This allows: 3.7.4 for "max_py_majver=3" and "max_py_majver=3 max_py_minver=7" --- easybuild/easyblocks/generic/pythonpackage.py | 22 ++++++++++--------- test/easyblocks/module.py | 20 ++++++++++++----- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/easybuild/easyblocks/generic/pythonpackage.py b/easybuild/easyblocks/generic/pythonpackage.py index b78868a2b4..8b651e44aa 100644 --- a/easybuild/easyblocks/generic/pythonpackage.py +++ b/easybuild/easyblocks/generic/pythonpackage.py @@ -110,7 +110,7 @@ def check_python_cmd(python_cmd): log.debug(f"Python command '{python_cmd}' not available through $PATH") return False - pyver = det_python_version(python_cmd) + pyver = LooseVersion(det_python_version(python_cmd)) if req_maj_ver is not None: if req_min_ver is None: @@ -119,26 +119,28 @@ def check_python_cmd(python_cmd): req_majmin_ver = '%s.%s' % (req_maj_ver, req_min_ver) # (strict) check for major version - maj_ver = pyver.split('.')[0] - if maj_ver != str(req_maj_ver): + maj_ver = pyver.version[0] + if maj_ver != req_maj_ver: log.debug(f"Major Python version does not match: {maj_ver} vs {req_maj_ver}") return False # check for minimal minor version - if LooseVersion(pyver) < LooseVersion(req_majmin_ver): + if pyver < req_majmin_ver: log.debug(f"Minimal requirement for minor Python version not satisfied: {pyver} vs {req_majmin_ver}") return False if max_py_majver is not None: if max_py_minver is None: - max_majmin_ver = '%s.0' % max_py_majver + max_ver = int(max_py_majver) + tested_pyver = pyver.version[0] else: - max_majmin_ver = '%s.%s' % (max_py_majver, max_py_minver) + max_ver = LooseVersion('%s.%s' % (max_py_majver, max_py_minver)) + # Make sure we test only until the minor version, because 3.9.3 > 3.9 but we want to allow this + tested_pyver = '.'.join(str(v) for v in pyver.version[:2]) - if LooseVersion(pyver) > LooseVersion(max_majmin_ver): - log.debug("Python version (%s) on the system is newer than the maximum supported " - "Python version specified in the easyconfig (%s)", - pyver, max_majmin_ver) + if tested_pyver > max_ver: + log.debug(f"Python version ({pyver}) on the system is newer than the maximum supported " + f"Python version specified in the easyconfig ({max_ver})") return False # all check passed diff --git a/test/easyblocks/module.py b/test/easyblocks/module.py index 7b9c6d612f..f35ff3cd7d 100644 --- a/test/easyblocks/module.py +++ b/test/easyblocks/module.py @@ -229,12 +229,20 @@ def test_pythonpackage_det_pylibdir(self): def test_pythonpackage_pick_python_cmd(self): """Test pick_python_cmd function from pythonpackage.py.""" from easybuild.easyblocks.generic.pythonpackage import pick_python_cmd - self.assertTrue(pick_python_cmd() is not None) - self.assertTrue(pick_python_cmd(3) is not None) - self.assertTrue(pick_python_cmd(3, 6) is not None) - self.assertTrue(pick_python_cmd(123, 456) is None) - self.assertTrue(pick_python_cmd(2, 6, 123, 456) is not None) - self.assertTrue(pick_python_cmd(2, 6, 1, 1) is None) + self.assertIsNotNone(pick_python_cmd()) + self.assertIsNotNone(pick_python_cmd(3)) + self.assertIsNotNone(pick_python_cmd(3, 6)) + self.assertIsNone(pick_python_cmd(123, 456)) + self.assertIsNotNone(pick_python_cmd(2, 6, 123, 456)) + self.assertIsNotNone(pick_python_cmd(2, 6, 2)) + self.assertIsNone(pick_python_cmd(2, 6, 1, 1)) + maj_ver, min_ver = sys.version_info[0:2] + self.assertIsNotNone(pick_python_cmd(maj_ver, min_ver)) + self.assertIsNotNone(pick_python_cmd(maj_ver, min_ver, max_py_majver=maj_ver)) + self.assertIsNotNone(pick_python_cmd(maj_ver, min_ver, max_py_majver=maj_ver, max_py_minver=min_ver)) + self.assertIsNotNone(pick_python_cmd(maj_ver, min_ver, max_py_majver=maj_ver, max_py_minver=min_ver + 1)) + self.assertIsNone(pick_python_cmd(maj_ver, min_ver, max_py_majver=maj_ver - 1)) + self.assertIsNone(pick_python_cmd(maj_ver, min_ver, max_py_majver=maj_ver, max_py_minver=min_ver - 1)) def template_module_only_test(self, easyblock, name, version='1.3.2', extra_txt='', tmpdir=None):