From 6f6722c4c4fb1c4f8e58fe51b57af9a351ebd1dc Mon Sep 17 00:00:00 2001 From: Chris McBride <3595025+ankona@users.noreply.github.com> Date: Wed, 31 Jul 2024 19:23:49 -0400 Subject: [PATCH 1/4] Mitigate dragon/numpy, mypy/typing_extension dependency issues (#653) This PR mitigates two issues encountered during installation on build agents ## mypy/typing_extensions Installation of mypy or dragon in separate build actions caused some dependencies (typing_extensions, numpy) to be upgraded. Those upgrades result in runtime failures. The build actions were tweaked to allow pip to consider all optional dependencies during resolution. ## dragon/numpy Additionally, the numpy version was capped on dragon installations. [ committed by @ankona] [ approved by @ashao @MattToast ] --- .github/workflows/run_tests.yml | 3 +-- doc/changelog.md | 7 +++++++ smartsim/_core/_cli/scripts/dragon_install.py | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/.github/workflows/run_tests.yml b/.github/workflows/run_tests.yml index f3a97474d..bf3ceefc3 100644 --- a/.github/workflows/run_tests.yml +++ b/.github/workflows/run_tests.yml @@ -109,7 +109,7 @@ jobs: - name: Install SmartSim (with ML backends) run: | python -m pip install git+https://github.com/CrayLabs/SmartRedis.git@develop#egg=smartredis - python -m pip install .[dev,ml] + python -m pip install .[dev,mypy,ml] - name: Install ML Runtimes with Smart (with pt, tf, and onnx support) if: contains( matrix.os, 'ubuntu' ) || contains( matrix.os, 'macos-12') @@ -121,7 +121,6 @@ jobs: - name: Run mypy run: | - python -m pip install .[mypy] make check-mypy - name: Run Pylint diff --git a/doc/changelog.md b/doc/changelog.md index 6efeedfaf..61c8a8779 100644 --- a/doc/changelog.md +++ b/doc/changelog.md @@ -15,6 +15,7 @@ To be released at some future point in time Description +- Mitigate dependency installation issues - Fix internal host name representation for Dragon backend - Make dependencies more discoverable in setup.py - Add hardware pinning capability when using dragon @@ -27,6 +28,12 @@ Description Detailed Notes +- Installation of mypy or dragon in separate build actions caused + some dependencies (typing_extensions, numpy) to be upgraded and + caused runtime failures. The build actions were tweaked to include + all optional dependencies to be considered by pip during resolution. + Additionally, the numpy version was capped on dragon installations. + ([SmartSim-PR653](https://github.com/CrayLabs/SmartSim/pull/653)) - setup.py used to define dependencies in a way that was not amenable to code scanning tools. Direct dependencies now appear directly in the setup call and the definition of the SmartRedis version diff --git a/smartsim/_core/_cli/scripts/dragon_install.py b/smartsim/_core/_cli/scripts/dragon_install.py index 466c390bd..a2e8ed36f 100644 --- a/smartsim/_core/_cli/scripts/dragon_install.py +++ b/smartsim/_core/_cli/scripts/dragon_install.py @@ -182,7 +182,7 @@ def install_package(asset_dir: pathlib.Path) -> int: logger.info(f"Installing package: {wheel_path.absolute()}") try: - pip("install", "--force-reinstall", str(wheel_path)) + pip("install", "--force-reinstall", str(wheel_path), "numpy<2") wheel_path = next(wheels, None) except Exception: logger.error(f"Unable to install from {asset_dir}") From fde9f2e9aba39ff53bd34db38e91cbca103f37e1 Mon Sep 17 00:00:00 2001 From: Andrew Shao Date: Tue, 6 Aug 2024 09:47:26 -0700 Subject: [PATCH 2/4] Remove builder from setup.py (#654) The builder module was included in `setup.py` to allow us to ship the main Redis binaries (not RedisAI) with installs from PyPI. The changes in this PR remove our ability to do this and requires users to build Redis as part of the `smart build`. This change in behaviour was deemed reasonable to allow for easier maintenance and extension of the Builder class as well as simplify the deployment of wheels. [ committed by @ashao ] [ reviewed by @MattToast ] --- doc/changelog.md | 6 ++++++ setup.py | 53 ------------------------------------------------ 2 files changed, 6 insertions(+), 53 deletions(-) diff --git a/doc/changelog.md b/doc/changelog.md index 61c8a8779..83e52fd68 100644 --- a/doc/changelog.md +++ b/doc/changelog.md @@ -15,6 +15,7 @@ To be released at some future point in time Description +- Remove build of Redis from setup.py - Mitigate dependency installation issues - Fix internal host name representation for Dragon backend - Make dependencies more discoverable in setup.py @@ -28,6 +29,11 @@ Description Detailed Notes +- The builder module was included in setup.py to allow us to ship the + main Redis binaries (not RedisAI) with installs from PyPI. To + allow easier maintenance of this file and enable future complexity + this has been removed. The Redis binaries will thus be built + by users during the `smart build` step - Installation of mypy or dragon in separate build actions caused some dependencies (typing_extensions, numpy) to be upgraded and caused runtime failures. The build actions were tweaked to include diff --git a/setup.py b/setup.py index cd8eabec1..328bf1ffb 100644 --- a/setup.py +++ b/setup.py @@ -77,9 +77,6 @@ from pathlib import Path from setuptools import setup -from setuptools.command.build_py import build_py -from setuptools.command.install import install -from setuptools.dist import Distribution # Some necessary evils we have to do to be able to use # the _install tools in smartsim/smartsim/_core/_install @@ -95,12 +92,6 @@ buildenv = importlib.util.module_from_spec(buildenv_spec) buildenv_spec.loader.exec_module(buildenv) -# import builder module -builder_path = _install_dir.joinpath("builder.py") -builder_spec = importlib.util.spec_from_file_location("builder", str(builder_path)) -builder = importlib.util.module_from_spec(builder_spec) -builder_spec.loader.exec_module(builder) - # helper classes for building dependencies that are # also utilized by the Smart CLI build_env = buildenv.BuildEnv(checks=False) @@ -128,47 +119,8 @@ class BuildError(Exception): pass - -# Hacky workaround for solving CI build "purelib" issue -# see https://github.com/google/or-tools/issues/616 -class InstallPlatlib(install): - def finalize_options(self): - super().finalize_options() - if self.distribution.has_ext_modules(): - self.install_lib = self.install_platlib - - -class SmartSimBuild(build_py): - def run(self): - database_builder = builder.DatabaseBuilder( - build_env(), build_env.MALLOC, build_env.JOBS - ) - if not database_builder.is_built: - database_builder.build_from_git(versions.REDIS_URL, versions.REDIS) - - database_builder.cleanup() - - # run original build_py command - super().run() - - -# Tested with wheel v0.29.0 -class BinaryDistribution(Distribution): - """Distribution which always forces a binary package with platform name - - We use this because we want to pre-package Redis for certain - platforms to use. - """ - - def has_ext_modules(_placeholder): - return True - - # Define needed dependencies for the installation -# Add SmartRedis at specific version -# install_requires.append("smartredis>={}".format(versions.SMARTREDIS)) - extras_require = { "dev": [ "black==24.1a1", @@ -232,13 +184,8 @@ def has_ext_modules(_placeholder): "numpy<2", "smartredis>=0.5,<0.6", ], - cmdclass={ - "build_py": SmartSimBuild, - "install": InstallPlatlib, - }, zip_safe=False, extras_require=extras_require, - distclass=BinaryDistribution, entry_points={ "console_scripts": [ "smart = smartsim._core._cli.__main__:main", From 6abbd77bcec78a0a24a375cbcfa084f411b6b13f Mon Sep 17 00:00:00 2001 From: Matt Ellis Date: Wed, 7 Aug 2024 15:39:30 -0700 Subject: [PATCH 3/4] Update codecov to v4.5.0 (#657) The version of codecov has been updated to v4.5.0 for the github actions. [ committed by @mellis13 ] [ reviewed by @amandarichardsonn ] --- .github/workflows/run_tests.yml | 2 +- doc/changelog.md | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/run_tests.yml b/.github/workflows/run_tests.yml index bf3ceefc3..1f0b729ed 100644 --- a/.github/workflows/run_tests.yml +++ b/.github/workflows/run_tests.yml @@ -164,7 +164,7 @@ jobs: retention-days: 5 - name: Upload Pytest coverage to Codecov - uses: codecov/codecov-action@v3.1.4 + uses: codecov/codecov-action@v4.5.0 with: fail_ci_if_error: false files: ./coverage.xml diff --git a/doc/changelog.md b/doc/changelog.md index 83e52fd68..740197ce5 100644 --- a/doc/changelog.md +++ b/doc/changelog.md @@ -15,6 +15,7 @@ To be released at some future point in time Description +- Update codecov to 4.5.0 - Remove build of Redis from setup.py - Mitigate dependency installation issues - Fix internal host name representation for Dragon backend @@ -29,6 +30,8 @@ Description Detailed Notes +- Update codecov to 4.5.0 to mitigate GitHub action failure + ([SmartSim-PR657](https://github.com/CrayLabs/SmartSim/pull/657)) - The builder module was included in setup.py to allow us to ship the main Redis binaries (not RedisAI) with installs from PyPI. To allow easier maintenance of this file and enable future complexity From bf348a0a8a2e950bb5031e4bfbcc41f29e45f51d Mon Sep 17 00:00:00 2001 From: Julia Putko Date: Thu, 15 Aug 2024 11:33:53 -0500 Subject: [PATCH 4/4] remove where install, build_py and Distribution were used in setup.py --- setup.py | 37 +------------------------------------ 1 file changed, 1 insertion(+), 36 deletions(-) diff --git a/setup.py b/setup.py index 656a74152..328bf1ffb 100644 --- a/setup.py +++ b/setup.py @@ -119,41 +119,6 @@ class BuildError(Exception): pass - -# Hacky workaround for solving CI build "purelib" issue -# see https://github.com/google/or-tools/issues/616 -class InstallPlatlib(install): - def finalize_options(self): - super().finalize_options() - if self.distribution.has_ext_modules(): - self.install_lib = self.install_platlib - - -class SmartSimBuild(build_py): - def run(self): - feature_store_builder = builder.FeatureStoreBuilder( - build_env(), build_env.MALLOC, build_env.JOBS - ) - if not feature_store_builder.is_built: - feature_store_builder.build_from_git(versions.REDIS_URL, versions.REDIS) - - feature_store_builder.cleanup() - - # run original build_py command - super().run() - - -# Tested with wheel v0.29.0 -class BinaryDistribution(Distribution): - """Distribution which always forces a binary package with platform name - - We use this because we want to pre-package Redis for certain - platforms to use. - """ - - def has_ext_modules(_placeholder): - return True - # Define needed dependencies for the installation extras_require = { @@ -174,6 +139,7 @@ def has_ext_modules(_placeholder): "types-tqdm", "types-tensorflow==2.12.0.9", "types-setuptools", + "typing_extensions>=4.1.0", ], "docs": [ "Sphinx==6.2.1", @@ -217,7 +183,6 @@ def has_ext_modules(_placeholder): "pygithub>=2.3.0", "numpy<2", "smartredis>=0.5,<0.6", - "typing_extensions>=4.1.0,<4.6", ], zip_safe=False, extras_require=extras_require,