Code review of main
Merge request reports
Activity
- src/mpsd_software_manager/__init__.py 0 → 100644
1 """Command-line tool to install software using Spack.""" 2 3 import sys 4 from logging import getLogger 5 from typing import Any 6 7 from ._version import __version__ as __version__ The redundant import is suggested in https://typing.readthedocs.io/en/latest/spec/distributing.html#import-conventions and https://docs.astral.sh/ruff/rules/unused-import/#why-is-this-bad. I would prefer it over strings in
__all__
because it is easier to spot typos or missing re-exports.
- src/mpsd_software_manager/__init__.py 0 → 100644
1 """Command-line tool to install software using Spack.""" 2 3 import sys 4 from logging import getLogger 5 from typing import Any 6 7 from ._version import __version__ as __version__ 8 9 logger = getLogger(__name__) 10 11 12 def silent_keyboard_interrupt( - src/mpsd_software_manager/config.py 0 → 100644
8 ... 9 Config().<property> 10 11 The config object should be initialised in the command line module. 12 """ 13 14 from __future__ import annotations 15 16 from dataclasses import InitVar, dataclass, field 17 from logging import getLogger 18 from pathlib import Path 19 from typing import Any 20 21 from .util import abort, detect_cpu_microarch 22 23 MPSD_SOFTWARE_ROOT_MARKER: str = ".mpsd-software-root" - src/mpsd_software_manager/config.py 0 → 100644
13 14 from __future__ import annotations 15 16 from dataclasses import InitVar, dataclass, field 17 from logging import getLogger 18 from pathlib import Path 19 from typing import Any 20 21 from .util import abort, detect_cpu_microarch 22 23 MPSD_SOFTWARE_ROOT_MARKER: str = ".mpsd-software-root" 24 25 logger = getLogger(__name__) 26 27 28 class Singleton(type): - src/mpsd_software_manager/init.py 0 → 100644
6 import datetime 7 from logging import getLogger 8 from pathlib import Path 9 10 import mpsd_software_manager as smgr 11 12 from .config import MPSD_SOFTWARE_ROOT_MARKER 13 14 logger = getLogger(__name__) 15 16 17 def init(_args: argparse.Namespace) -> None: 18 """Initialise directory as mpsd software root. 19 20 - create .mpsd-software-root file as a marker 21 - initialise logging I don't understand how the logging is initialized here.
Edited by Victor László Horváth
- src/mpsd_software_manager/log_handler.py 0 → 100644
7 8 from rich.logging import RichHandler 9 10 from .config import Config 11 12 logger = logging.getLogger(__name__) 13 14 15 def init_stdout_logger(verbose: bool = False) -> None: 16 """Show log messages on stdout.""" 17 level = logging.DEBUG if verbose else logging.INFO 18 logging.basicConfig( 19 format="%(message)s", 20 handlers=[RichHandler(level=level, show_time=False, show_path=False)], 21 ) 22 # set only interal logging to a low level, ignore third party - src/mpsd_software_manager/log_handler.py 0 → 100644
23 logging.getLogger("mpsd_software_manager").setLevel(level) 24 25 26 def init_file_logger() -> None: 27 """Write log messages to a file.""" 28 config = Config() 29 file_name = ( 30 f"{config.microarch}-{datetime.datetime.now().isoformat(timespec='seconds')}" 31 ) 32 config.log_root.mkdir(parents=True, exist_ok=True) 33 log_file = config.log_root / file_name 34 if log_file.exists(): 35 logger.error("Log file '%s' exists already; skipping file logging", log_file) 36 return 37 else: 38 logger.info("Detailed log written to '%s'", log_file) - src/mpsd_software_manager/config.py 0 → 100644
54 55 release_root: Path = field(init=False) 56 microarch_root: Path = field(init=False) 57 spack_root: Path = field(init=False) 58 lmod_root: Path = field(init=False) 59 spack_environments_root: Path = field(init=False) 60 log_root: Path = field(init=False) 61 62 source_cache_root: Path = Path("/opt_mpsd/mpsd_spack_sources") 63 binary_cache_root: Path = field(init=False) 64 spack_user_cache_root: Path = field(init=False) 65 66 system_compiler: str = field(init=False) 67 68 spack_environments_repository = ("gitlab.gwdg.de", "mpsd-cs/spack-environments") 69 spack_repository = ("github.com", "mpsd-computational-science/spack") - Comment on lines +68 to +69
Maybe
68 spack_environments_repository = ("gitlab.gwdg.de", "mpsd-cs/spack-environments") 69 spack_repository = ("github.com", "mpsd-computational-science/spack") 68 spack_environments_repository = Repo(host="gitlab.gwdg.de", repo_name="mpsd-cs/spack-environments") 69 spack_repository = Repo(host="github.com", repo_name="mpsd-computational-science/spack") using
namedtuple
or similar. When looking at the config itself it is obvious what the parts of the tuple are meant for but when looking at code likeconfig.spack_repository[0]
it requires extra work to understand what is happening.
- src/mpsd_software_manager/status.py 0 → 100644
13 14 logger = getLogger(__name__) 15 16 17 def status(args: argparse.Namespace) -> None: 18 """Status of installed releases and package sets.""" 19 require_valid_software_root(command="status") 20 21 if args.software_release is None: 22 mpsd_software_status() 23 elif args.package_set is None: 24 release_status(args.software_release) 25 else: 26 package_set_status( 27 args.software_release, args.package_set, args.package_details 28 ) - Comment on lines +17 to +28
- src/mpsd_software_manager/install.py 0 → 100644
28 29 require_valid_software_root(command="install") 30 spack_environments_branch = validate_software_release_and_package_sets( 31 software_release, package_sets 32 ) 33 34 config = Config() 35 config.resolve_paths(software_release) 36 config.source_cache_root = args.source_cache_path 37 38 init_file_logger() 39 40 spack.ensure_no_active_spack() 41 42 # logger.critical("git clones disabled") 43 initialise_dir(config.release_root, "release") - Comment on lines +40 to +43
- src/mpsd_software_manager/install.py 0 → 100644
14 from .config import Config, require_valid_software_root 15 from .git_helper import initialise_or_update_repository 16 from .log_handler import init_file_logger 17 from .util import abort, initialise_dir 18 19 logger = getLogger(__name__) 20 21 22 def install(args: argparse.Namespace) -> None: 23 """Install package set(s).""" 24 software_release: str = args.software_release 25 package_sets: list[str] = args.package_set 26 27 # print(args) 28 29 require_valid_software_root(command="install") - Comment on lines +25 to +29
- src/mpsd_software_manager/spack.py 0 → 100644
147 if replacements: 148 for placeholder, value in replacements: 149 content = content.replace(placeholder, value) 150 151 dest.parent.mkdir(parents=True, exist_ok=True) 152 with dest.open("w") as f: 153 f.write(content) 154 155 156 def add_mirror(type_: str, name: str, path: Path) -> None: 157 """Add mirror with given name if it is not yet configured and the path exists.""" 158 if not path.is_dir(): 159 logger.warning("Ignoring mirror '%s', '%s' is not a directory", name, path) 160 return 161 existing_mirrors = spack("mirror list").stdout 162 if name in existing_mirrors: - src/mpsd_software_manager/spack.py 0 → 100644
45 is not None 46 ): 47 # compiler path: cc = /path/to/spack/root -> no system compiler 48 continue 49 system_compilers.append(compiler) 50 51 if len(system_compilers) == 0: 52 abort("Could not find a system compiler.") 53 elif ( 54 requested_system_compiler is not None 55 and requested_system_compiler not in system_compilers 56 ): 57 abort( 58 f"Requested system compiler '{requested_system_compiler}' is not known" 59 " to Spack" 60 ) - Comment on lines +57 to +60
- src/mpsd_software_manager/spack.py 0 → 100644
60 ) 61 elif requested_system_compiler is None and len(system_compilers) > 1: 62 abort( 63 f"Found multiple system compilers: '{system_compilers}'; specify preferred" 64 " system compiler on command line" 65 ) 66 67 if requested_system_compiler is not None: 68 logger.debug( 69 "Using '%s' as system compiler per user request", requested_system_compiler 70 ) 71 config.system_compiler = requested_system_compiler 72 else: 73 system_compiler = system_compilers[0] 74 logger.debug("Found system compiler '%s'", system_compiler) 75 config.system_compiler = system_compiler - Comment on lines +34 to +75
In general, some of the code logic in the whole file could be separated into "spack interaction logic" and "command/user interface logic", in this case for example:
46 def find_system_compiler(requested_system_compiler: str | None) -> None: 47 """Auto-detect system compiler or use user-specified compiler.""" 48 logger.info("spack: detecting system compiler") 49 spack("compiler find --scope site", log_callback=logger.debug) 50 spack_compilers = spack("compiler list", logger.debug).stdout 51 config = Config() 52 system_compilers = [] 53 for compiler in re.findall(r"\n(\w+@[0-9.]+)", spack_compilers): 54 compiler_info = spack(f"compiler info {compiler}").stdout 55 if ( 56 re.search(rf"\n\s*cc\s*=\s*{config.spack_root!s}", compiler_info) 57 is not None 58 ): 59 # compiler path: cc = /path/to/spack/root -> no system compiler 60 continue 61 system_compilers.append(compiler) 62 63 if len(system_compilers) == 0: 64 abort("Could not find a system compiler.") 65 elif ( 66 requested_system_compiler is not None 67 and requested_system_compiler not in system_compilers 68 ): 69 abort( 70 f"Requested system compiler '{requested_system_compiler}' is not known" 71 " to Spack" 72 ) 73 elif requested_system_compiler is None and len(system_compilers) > 1: 74 abort( 75 f"Found multiple system compilers: '{system_compilers}'; specify preferred" 76 " system compiler on command line" 77 ) 78 79 if requested_system_compiler is not None: 80 logger.debug( 81 "Using '%s' as system compiler per user request", requested_system_compiler 82 ) 83 config.system_compiler = requested_system_compiler 84 else: 85 system_compiler = system_compilers[0] 86 logger.debug("Found system compiler '%s'", system_compiler) 87 config.system_compiler = system_compiler 46 def find_system_compilers() -> list[str]: 47 spack("compiler find --scope site", log_callback=logger.debug) 48 spack_compilers = spack("compiler list", logger.debug).stdout 49 config = Config() 50 system_compilers = [] 51 for compiler in re.findall(r"\n(\w+@[0-9.]+)", spack_compilers): 52 compiler_info = spack(f"compiler info {compiler}").stdout 53 if ( 54 re.search(rf"\n\s*cc\s*=\s*{config.spack_root!s}", compiler_info) 55 is not None 56 ): 57 # compiler path: cc = /path/to/spack/root -> no system compiler 58 continue 59 system_compilers.append(compiler) 60 return system_compilers 61 62 def set_system_compiler(requested_system_compiler: str | None) -> None: 63 """Auto-detect system compiler or use user-specified compiler.""" 64 logger.info("spack: detecting system compiler") 65 system_compilers = find_system_compilers() 66 67 if len(system_compilers) == 0: 68 abort("Could not find a system compiler.") 69 elif ( 70 requested_system_compiler is not None 71 and requested_system_compiler not in system_compilers 72 ): 73 abort( 74 f"Requested system compiler '{requested_system_compiler}' is not known" 75 " to Spack" 76 ) 77 elif requested_system_compiler is None and len(system_compilers) > 1: 78 abort( 79 f"Found multiple system compilers: '{system_compilers}'; specify preferred" 80 " system compiler on command line" 81 ) 82 83 if requested_system_compiler is not None: 84 logger.debug( 85 "Using '%s' as system compiler per user request", requested_system_compiler 86 ) 87 config.system_compiler = requested_system_compiler 88 else: 89 system_compiler = system_compilers[0] 90 logger.debug("Found system compiler '%s'", system_compiler) 91 config.system_compiler = system_compiler But considering that this python package is a scripting approach and probably not a constantly growing software aiming for a lot of re-usability is not the primary goal here...
- src/mpsd_software_manager/spack.py 0 → 100644
65 ) 66 67 if requested_system_compiler is not None: 68 logger.debug( 69 "Using '%s' as system compiler per user request", requested_system_compiler 70 ) 71 config.system_compiler = requested_system_compiler 72 else: 73 system_compiler = system_compilers[0] 74 logger.debug("Found system compiler '%s'", system_compiler) 75 config.system_compiler = system_compiler 76 77 78 def update_custom_spack_config() -> None: 79 """Apply custom spack configuration from spack-environments repository.""" 80 # copy changed this line in version 14 of the diff
- src/mpsd_software_manager/spack.py 0 → 100644
181 182 183 def install_package_set_from_file(package_set: str) -> None: 184 """Install list of packages globally.""" 185 package_list = ( 186 Config().spack_environments_root 187 / "toolchains" 188 / package_set 189 / "global_packages.list" 190 ) 191 logger.info("Installing packages from %s", package_list) 192 with package_list.open() as f: 193 package_str = f.read() 194 195 packages = [] 196 for package in package_str.replace("\\\n", "").split("\n"): - src/mpsd_software_manager/spack.py 0 → 100644
524 command: str, log_callbacks: dict[str, Callable[[str], None]], **kwargs: Any 525 ) -> None: 526 """Run spack command and write stdout to log callback asynchronuously. 527 528 Implementation for asyncio code based on: 529 https://kevinmccarthy.org/2016/07/25/streaming-subprocess-stdin-and-stdout-with-asyncio-in-python/ 530 """ 531 loop = asyncio.get_event_loop() 532 returncode = loop.run_until_complete( 533 run_async_spack_shell_command( 534 f"spack {command}", 535 log_callbacks=log_callbacks, 536 env=spack_shell_env(), 537 **kwargs, 538 ) 539 ) - Comment on lines +531 to +539
Is there a specific reason to use
asyncio.get_event_loop
andloop.run_until_complete
instead ofasyncio.run
? The documentation ofasyncio
states that the event loop is considered a low-level API (including a deprecation warning regardingget_event_loop
) and applications should typically use the high-level functions (https://docs.python.org/3/library/asyncio-eventloop.html).Edited by Victor László Horváth
- src/mpsd_software_manager/spack.py 0 → 100644
372 Config().lmod_root / mpi_module / compiler_module / "octopus-dependencies" 373 ) 374 375 write_lua_module(lmod_octopus_dependencies_root / "full.lua", octopus_dependencies) 376 377 378 def write_lua_module(file_path: Path, modules: list[str]) -> None: 379 """Write lua metamodule with a list of dependencies; create directory if needed.""" 380 logger.debug("writing lmod module file '%s'", file_path) 381 file_path.parent.mkdir(exist_ok=True) 382 with file_path.open("w") as f: 383 f.write("-- -*- lua -*-\n") 384 f.write("\n".join(f'depends_on("{module}")' for module in modules)) 385 386 387 def install_toolchain_compiler(spack_environment: str) -> dict[str, dict[str, str]]: Preferably use dataclasses or similar for the return result here (at least for the values of the returned dict, as far as I see they contain only
package
andname
). Asdefault
andfallback
are the only used values in the dict overall this could be a dataclass too (at least if there is no plan to extend this part in the near future).Overall, the flow of this function is quite confusing.
- src/mpsd_software_manager/spack.py 0 → 100644
434 } 435 436 logger.debug("Required toolchain compiler: %s", compilers) 437 438 for compiler_definition in compilers.values(): 439 spack_install_package(compiler_definition["package"]) 440 compiler_path = spack( 441 f"location -i {compiler_definition['package']}" 442 ).stdout.strip() 443 logger.debug( 444 spack(f"compiler find --scope site {compiler_path}").stdout.strip() 445 ) 446 447 if len(compilers) == 2: 448 gcc = compilers["fallback"] 449 intel = compilers["default"] - Comment on lines +447 to +449
changed this line in version 32 of the diff
- src/mpsd_software_manager/spack.py 0 → 100644
307 with (spack_environment_path(spack_environment) / "loads").open() as f: 308 loads_file = f.read() 309 310 MPI_MODULES = ["openmpi", "intel-oneapi-mpi"] 311 GENERIC_TOOLCHAIN_MODULES = [ 312 "binutils", 313 "fftw", 314 "openblas", 315 "autoconf", 316 "libtool", 317 "automake", 318 "netlib-scalapack", 319 "cmake", 320 "ninja", 321 "intel-oneapi-mkl", 322 ]