Refactor tests and protobuf building for Python SDK (#1418)

* Fix Makefile driven proto building

Signed-off-by: Willem Pienaar <git@willem.co>

* Remove commented out files

Signed-off-by: Willem Pienaar <git@willem.co>

* Use Python 3.7 for unit tests

Signed-off-by: Willem Pienaar <git@willem.co>

* Refactor dependency management

Signed-off-by: Willem Pienaar <git@willem.co>

* Fix Telemetry tests

Signed-off-by: Willem Pienaar <git@willem.co>

* Remove unused test

Signed-off-by: Willem Pienaar <git@willem.co>

.github/workflows/integration_tests.yml

@@ -8,7 +8,7 @@ on:
 jobs:
   integration-test-python:
     runs-on: ubuntu-latest
-    container: gcr.io/kf-feast/feast-ci:latest
+    container: python:3.7
     steps:
       - uses: actions/checkout@v2
       - name: Set up Cloud SDK
@@ -21,9 +21,5 @@ jobs:
         run: gcloud info
       - name: Install dependencies
         run: make install-python-ci-dependencies
-      - name: Compile protos
-        run: make compile-protos-python
-      - name: Install python
-        run: make install-python
       - name: Test python
         run: FEAST_TELEMETRY=False pytest --verbose --color=yes sdk/python/tests --integration

.github/workflows/linter.yml

@@ -4,14 +4,12 @@ on: [push, pull_request]
 
 jobs:
   lint-python:
-    container: gcr.io/kf-feast/feast-ci:latest
+    container: python:3.7
     runs-on: [ubuntu-latest]
     steps:
       - uses: actions/checkout@v2
       - name: Install dependencies
         run: make install-python-ci-dependencies
-      - name: Compile protos
-        run: make compile-protos-python
       - name: Lint python
         run: make lint-python
 

.github/workflows/pr_integration_tests.yml

@@ -13,7 +13,7 @@ jobs:
     if: (github.event.action == 'labeled' && (github.event.label.name == 'approved' || github.event.label.name == 'ok-to-test'))
       || (github.event.action != 'labeled' && (contains(github.event.pull_request.labels.*.name, 'ok-to-test') || contains(github.event.pull_request.labels.*.name, 'approved')))
     runs-on: ubuntu-latest
-    container: gcr.io/kf-feast/feast-ci:latest
+    container: python:3.7
     steps:
       - uses: actions/checkout@v2
         with:
@@ -32,9 +32,5 @@ jobs:
         run: gcloud info
       - name: Install dependencies
         run: make install-python-ci-dependencies
-      - name: Compile protos
-        run: make compile-protos-python
-      - name: Install python
-        run: make install-python
       - name: Test python
         run: FEAST_TELEMETRY=False pytest --verbose --color=yes sdk/python/tests --integration

.github/workflows/unit_tests.yml

@@ -5,15 +5,11 @@ on: [push, pull_request]
 jobs:
   unit-test-python:
     runs-on: ubuntu-latest
-    container: gcr.io/kf-feast/feast-ci:latest
+    container: python:3.7
     steps:
       - uses: actions/checkout@v2
       - name: Install dependencies
         run: make install-python-ci-dependencies
-      - name: Compile protos
-        run: make compile-protos-python
-      - name: Install Python
-        run: make install-python
       - name: Test Python
         run: make test-python
 

.gitignore

@@ -188,5 +188,4 @@ dmypy.json
 sdk/python/docs/source
 sdk/python/docs/html
 sdk/python/feast/protos/
-sdk/python/tensorflow_metadata
 sdk/go/protos/

.prow.yaml

@@ -64,13 +64,6 @@ presubmits:
   branches:
   - ^v0\.(3|4)-branch$
 
-- name: test-python-sdk
-  decorate: true
-  spec:
-    containers:
-    - image: python:3.7
-      command: ["infra/scripts/test-python-sdk.sh"]
-
 - name: test-telemetry
   decorate: true
   run_if_changed: "sdk/python/.*"

.readthedocs.yml

@@ -9,6 +9,6 @@ formats:
 python:
   version: 3.7
   install:
-      - requirements: sdk/python/requirements-ci.txt
+      - requirements: sdk/python/docs/requirements.txt
       - path: sdk/python/
         method: setuptools

Makefile

@@ -39,13 +39,12 @@ install-ci-dependencies: install-python-ci-dependencies install-go-ci-dependenci
 # Python SDK
 
 install-python-ci-dependencies:
-	pip install --no-cache-dir -r sdk/python/requirements-ci.txt
+	pip install -e "sdk/python[ci]"
 
 package-protos:
 	cp -r ${ROOT_DIR}/protos ${ROOT_DIR}/sdk/python/feast/protos
 
 compile-protos-python:
-	pip install --ignore-installed mypy-protobuf || echo "Mypy could not be installed"
 	@$(foreach dir,$(PROTO_TYPE_SUBDIRS),cd ${ROOT_DIR}/protos; python -m grpc_tools.protoc -I. --grpc_python_out=../sdk/python/feast/protos/ --python_out=../sdk/python/feast/protos/ --mypy_out=../sdk/python/feast/protos/ feast/$(dir)/*.proto;)
 	@$(foreach dir,$(PROTO_TYPE_SUBDIRS),grep -rli 'from feast.$(dir)' sdk/python/feast/protos | xargs -I@ sed -i.bak 's/from feast.$(dir)/from feast.protos.feast.$(dir)/g' @;)
 	cd ${ROOT_DIR}/protos; python -m grpc_tools.protoc -I. --python_out=../sdk/python/ --mypy_out=../sdk/python/ tensorflow_metadata/proto/v0/*.proto

infra/scripts/test-telemetry.sh

@@ -5,10 +5,7 @@ set -e
 # Default artifact location setting in Prow jobs
 LOGS_ARTIFACT_PATH=/logs/artifacts
 
-pip install -r sdk/python/requirements-ci.txt
-make compile-protos-python
-
 cd sdk/python/
-pip install -e .
+pip install -e ".[ci]"
 cd telemetry_tests/
 pytest --junitxml=${LOGS_ARTIFACT_PATH}/python-sdk-test-report.xml

sdk/python/MANIFEST.in

@@ -0,0 +1 @@
+recursive-include feast/protos/ *.py

sdk/python/docs/requirements.txt

@@ -0,0 +1,5 @@
+grpcio-tools==1.31.0
+mypy==0.790
+mypy-protobuf==1.24
+firebase-admin==4.5.2
+google-cloud-datastore==2.1.0

sdk/python/setup.py

@@ -11,23 +11,21 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-import logging
+import glob
 import os
 import re
 import subprocess
 
+from distutils.cmd import Command
 from setuptools import find_packages
 
 try:
     from setuptools import setup
-    from setuptools.command.install import install
     from setuptools.command.develop import develop
-    from setuptools.command.egg_info import egg_info
-    from setuptools.command.sdist import sdist
-
+    from setuptools.command.build_py import build_py
 except ImportError:
     from distutils.core import setup
-    from distutils.command.install import install
+    from distutils.command.build_py import build_py
 
 NAME = "feast"
 DESCRIPTION = "Python SDK for Feast"
@@ -44,7 +42,7 @@
     "google-cloud-core==1.4.*",
     "googleapis-common-protos==1.52.*",
     "grpcio==1.31.0",
-    "Jinja2==2.11.3",
+    "Jinja2>=2.0.0",
     "pandas~=1.0.0",
     "pandavro==1.5.*",
     "protobuf>=3.10",
@@ -61,6 +59,33 @@
     "jsonschema",
 ]
 
+CI_REQUIRED = [
+    "cryptography==3.3.2",
+    "flake8",
+    "black==19.10b0",
+    "isort>=5",
+    "grpcio-tools==1.31.0",
+    "grpcio-testing==1.31.0",
+    "mock==2.0.0",
+    "moto",
+    "mypy==0.790",
+    "mypy-protobuf==1.24",
+    "avro==1.10.0",
+    "gcsfs",
+    "urllib3>=1.25.4",
+    "pytest==6.0.0",
+    "pytest-lazy-fixture==0.6.3",
+    "pytest-timeout==1.4.2",
+    "pytest-ordering==0.6.*",
+    "pytest-mock==1.10.4",
+    "Sphinx",
+    "sphinx-rtd-theme",
+    "adlfs==0.5.9",
+    "firebase-admin==4.5.2",
+    "google-cloud-datastore==2.1.0",
+    "pre-commit"
+]
+
 # README file from Feast repo root directory
 repo_root = (
     subprocess.Popen(["git", "rev-parse", "--show-toplevel"], stdout=subprocess.PIPE)
@@ -79,31 +104,62 @@
     r"^(?:[\/\w-]+)?(?P<version>[vV]?\d+(?:\.\d+){0,2}[^\+]*)(?:\+.*)?$"
 )
 
-proto_dirs = [x for x in os.listdir(repo_root + "/protos/feast")]
-proto_dirs.remove("third_party")
 
+class BuildProtoCommand(Command):
+    description = "Builds the proto files into python files."
 
-def pre_install_build():
-    subprocess.check_call("make compile-protos-python", shell=True, cwd=f"{repo_root}")
-    subprocess.check_call("make build-sphinx", shell=True, cwd=f"{repo_root}")
+    def initialize_options(self):
+        self.protoc = ["python", "-m", "grpc_tools.protoc"]  # find_executable("protoc")
+        self.proto_folder = os.path.join(repo_root, "protos")
+        self.this_package = os.path.join(os.path.dirname(__file__) or os.getcwd(), 'feast/protos')
+        self.sub_folders = ["core", "serving", "types", "storage"]
 
+    def finalize_options(self):
+        pass
 
-class CustomInstallCommand(install):
-    def do_egg_install(self):
-        pre_install_build()
-        install.do_egg_install(self)
+    def _generate_protos(self, path):
+        proto_files = glob.glob(os.path.join(self.proto_folder, path))
 
+        subprocess.check_call(self.protoc + [
+            '-I', self.proto_folder,
+            '--python_out', self.this_package,
+            '--grpc_python_out', self.this_package,
+            '--mypy_out', self.this_package] + proto_files)
 
-class CustomDevelopCommand(develop):
     def run(self):
-        pre_install_build()
-        develop.run(self)
+        for sub_folder in self.sub_folders:
+            self._generate_protos(f'feast/{sub_folder}/*.proto')
+
+        from pathlib import Path
+
+        for path in Path('feast/protos').rglob('*.py'):
+            for folder in self.sub_folders:
+                # Read in the file
+                with open(path, 'r') as file:
+                    filedata = file.read()
+
+                # Replace the target string
+                filedata = filedata.replace(f'from feast.{folder}', f'from feast.protos.feast.{folder}')
 
+                # Write the file out again
+                with open(path, 'w') as file:
+                    file.write(filedata)
+
+
+class BuildCommand(build_py):
+    """Custom build command."""
 
-class CustomEggInfoCommand(egg_info):
     def run(self):
-        pre_install_build()
-        egg_info.run(self)
+        self.run_command('build_proto')
+        build_py.run(self)
+
+
+class DevelopCommand(develop):
+    """Custom develop command."""
+
+    def run(self):
+        self.run_command('build_proto')
+        develop.run(self)
 
 
 setup(
@@ -114,14 +170,14 @@ def run(self):
     long_description_content_type="text/markdown",
     python_requires=REQUIRES_PYTHON,
     url=URL,
-    packages=find_packages(exclude=("tests",)) + ['.'],
+    packages=find_packages(exclude=("tests",)),
     install_requires=REQUIRED,
     # https://stackoverflow.com/questions/28509965/setuptools-development-requirements
     # Install dev requirements with: pip install -e .[dev]
     extras_require={
         "dev": ["mypy-protobuf==1.*", "grpcio-testing==1.*"],
-        "validation": ["great_expectations==0.13.2", "pyspark==3.0.1"],
         "docs": ["grpcio-tools"],
+        "ci": CI_REQUIRED
     },
     include_package_data=True,
     license="Apache",
@@ -135,7 +191,7 @@ def run(self):
     ],
     entry_points={"console_scripts": ["feast=feast.cli:cli"]},
     use_scm_version={"root": "../..", "relative_to": __file__, "tag_regex": TAG_REGEX},
-    setup_requires=["setuptools_scm", "grpcio-tools", "mypy-protobuf", "sphinx"],
+    setup_requires=["setuptools_scm", "grpcio", "grpcio-tools==1.31.0", "mypy-protobuf", "sphinx"],
     package_data={
         "": [
             "protos/feast/**/*.proto",
@@ -146,8 +202,8 @@ def run(self):
         ],
     },
     cmdclass={
-        "install": CustomInstallCommand,
-        "develop": CustomDevelopCommand,
-        "egg_info": CustomEggInfoCommand,
+        "build_proto": BuildProtoCommand,
+        "build_py": BuildCommand,
+        "develop": DevelopCommand,
     },
 )