Skip to content
Snippets Groups Projects

Code review of main

Open Victor László Horváth requested to merge main into dummy

Merge request reports

Pipeline #582748 failed

Pipeline failed for cfd7ed89 on main

Ready to merge by members who can write to the target branch.
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
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__
  • 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(
  • 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"
  • 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):
  • 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
  • 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
  • 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)
  • 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

      Suggested change
      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 like config.spack_repository[0] it requires extra work to understand what is happening.

    • Please register or sign in to reply
  • 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 )
  • 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
      Suggested change
      40 spack.ensure_no_active_spack()
      41
      42 # logger.critical("git clones disabled")
      43 initialise_dir(config.release_root, "release")
      40 spack.ensure_no_active_spack()
      41
      42 initialise_dir(config.release_root, "release")
    • Please register or sign in to reply
  • 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
      Suggested change
      25 package_sets: list[str] = args.package_set
      26
      27 # print(args)
      28
      29 require_valid_software_root(command="install")
      25 package_sets: list[str] = args.package_set
      26
      27 require_valid_software_root(command="install")
    • Please register or sign in to reply
  • 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:
    • I am not fully aware how the spack mirrors look like and if partial string matches are a problem here, for example when name = "stuff" and existing_mirrors = "stuff_with_prefix" name in existing_mirrors would yield True which may not be intended.

    • Please register or sign in to reply
  • 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 )
  • 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:

      Suggested change
      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...

    • Please register or sign in to reply
  • 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
  • 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"):
    • It is not trivial to understand what package_str.replace("\\\n", "").split("\n") is doing, maybe add a hint as a comment.

      Also, is it safe to assume that there are no other white spaces between \ and the newline (I am not aware of any syntax specification of these files)?

    • Please register or sign in to reply
  • 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 )
  • 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 and name). As default and fallback 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.

    • Please register or sign in to reply
  • 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"]
  • 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 ]
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading