feat(api): image building process improvement #404

naufalandika · 2025-04-08T03:19:42Z

Context

This PR contains improvement for pyfunc-ensembler-service / pyfunc-ensembler-job image building process. This improvement removes the need of supporting specific python versions and use the python version specified in user dependency instead, similar on how merlin builds pyfunc-server and batch-predictor images.

Changes in this PR includes improvement for the imagebuilder, pyfunc-ensembler-service / pyfunc-ensembler-job package, and little bit of sdk.

imagebuilder

Only use 1 base image for every python version used by the users.
Inject pyfunc-ensembler-service / pyfunc-ensembler-job dependency to user's conda.yaml.
Hash user conda.yaml to prevent the URL value of conda.yaml that has the same content to change and invalidating docker layer cache.

`pyfunc-ensembler-service` / `pyfunc-ensembler-job` package

Add new Makefile rule to build and publish the package to pypi
Add new github workflow to publish the package to pypi everytime sdk workflow is run successfully. pyfunc-ensembler-service / pyfunc-ensembler-job share the same version with turing-sdk package, but both of these package requires latest turing-sdk version, so we need to run the workflow only after sdk workflow completed.

sdk

Change the trigger to sdk workflow to python/* tag push instead of sdk/* tag push. python/* tag and version will be used by turing-sdk, pyfunc-ensembler-service and pyfunc-ensembler-job packages.

Main Modifications

imagebuilder/imagebuilder.go:

use 1 config for --build-arg=BASE_IMAGE=%s value
hash user's conda.yaml

engines/pyfunc-ensembler-*/Makefile:

add rule to build and publish pyfunc-ensembler-* package

engines/pyfunc-ensembler-*/app.Dockerfile:

injects turing-pyfunc-ensembler-* to user's dependency

.github/workflows/*

add publishing python/* tag as sdk workflow triggers
run pyfunc-ensembler-* workflow after sdk workflow is run successfully

…python on the app.Dockerfile instead

…on app.Dockerfile

deadlycoconuts

Thanks a lot for all the refactoring and testing work gone into this PR! 🚀🙏🏼 Most of it looks good but I just left a couple of small comments here and there (the comments on the ensembler job stuff generally apply to the ensembler server stuff too). Feel free to merge this PR once you've addressed the comments!

deadlycoconuts · 2025-04-22T09:21:34Z

.github/workflows/pyfunc-ensembler-job.yaml

-    strategy:
-      matrix:
-        python-version: ["3.8", "3.9", "3.10"]


Actually I think it's alright to keep this, so that we perform unit tests of the pyfunc ensembler job/service for different versions of Python 🤔

deadlycoconuts · 2025-04-22T09:21:56Z

.github/workflows/pyfunc-ensembler-job.yaml

+  workflow_run:
+    workflows: ["sdk"]
+    types:
+      - completed


Wow this is cool

But I can't test this until the workflow is merged to main tho, hopefully it runs as expected

deadlycoconuts · 2025-04-22T09:25:12Z

.github/workflows/pyfunc-ensembler-job.yaml

        || ( github.event_name != 'pull_request' )
-        || ( github.event.pull_request.head.repo.full_name == github.repository )
+        || ( github.event.pull_request.head.repo.full_name == github.repository )) &&
+        ${{ github.event.workflow_run.conclusion == 'success' }}


If I understand correctly, github.event.workflow_run refers to the SDK workflow right? If it is, then wouldn't the on.workflow_run.workflows thing you configured at the top of this file already guarantee that the workflow needs to be successful before this entire workflow can even begin running? 🤔 In other words, would ${{ github.event.workflow_run.conclusion == 'success' }} be redundant here?

workflow_run: workflows: ["sdk"] types: - completed

the completed in here doesn't check whether the conclusion of the workflow is success or failure, therefore I need to check the conclusion manually. The example on the documentation suggests that it work like that. Maybe my comment above the workflow_run is confusing

deadlycoconuts · 2025-04-22T09:36:35Z

.github/workflows/pyfunc-ensembler-job.yaml

+        run: |
+          set -o pipefail
+          make build-and-publish | tee output.log
+          echo "pyfunc-ensembler-job=$(sed -n 's%Building docker image: \(.*\)%\1%p' output.log)" >> $GITHUB_OUTPUT


Ooh, why do we need to store this as a GitHub output?

I can't remember my reason on doing this haha, maybe because the Build Docker Image job saves the log to the output, I also save this job's log to the output. But if it isn't necessary I can delete this part. And setting the output using ::set-output is deprecated and they ask us to setting it using >> $GITHUB_OUTPUT

deadlycoconuts · 2025-04-22T10:07:43Z

api/turing/imagebuilder/imagebuilder.go

 	artifactServiceType string
+	artifactService     artifact.Service


I believe we can simplify this struct by removing the artifactServiceType field because it is the same as artifactService.GetType().

Ah I missed that, thanks. Updated

deadlycoconuts · 2025-04-22T10:14:09Z

api/turing/imagebuilder/imagebuilder_test.go

-				tt.artifactURI,
+				testArtifactURI,


Lol thanks for realising that tt.artifactURI is the same for all test cases and for replacing it with a single re-usable variable instead.

deadlycoconuts · 2025-04-22T10:25:07Z

engines/pyfunc-ensembler-job/Dockerfile

+# Install yq
+ENV YQ_VERSION=v4.42.1
+RUN wget https://github.com/mikefarah/yq/releases/download/${YQ_VERSION}/yq_linux_amd64 -O /usr/bin/yq && \
+    chmod +x /usr/bin/yq
+


ultra nit: can we move this up above the block that installs the gcloud SDK? So it looks similar to the Dockerfile found in Merlin? 😅

hahaha sure no problem, updated!

deadlycoconuts · 2025-04-22T10:33:25Z

engines/pyfunc-ensembler-job/Makefile

-setup: $(CONDA_ENV_NAME)
-$(CONDA_ENV_NAME):
-	@conda env update -f env-$(PYTHON_VERSION).yaml -n $(CONDA_ENV_NAME) --prune
-	$(ACTIVATE_ENV) && pip install -r requirements.dev.txt
+setup: build
+	@pip install pipenv
+	@DIST_VERSION=$$(echo $(VERSION) | \
+		sed -E 's/^v([0-9]+\.[0-9]+\.[0-9]+)(-rc([0-9]+))?/\1rc\3/'); \
+		pipenv run pip install "dist/turing_pyfunc_ensembler_job-$${DIST_VERSION}-py3-none-any.whl[dev]"


Just wondering, did you change all the conda virtual env stuff to pipenv to follow Merlin's implementation? I was thinking if it was possible to just keep all the conda-related stuff 😅

yea I think I tried to mimic how merlin does this and they use pipenv to do this. Should I rewrite it to use conda ?

deadlycoconuts · 2025-04-22T10:36:12Z

engines/pyfunc-ensembler-job/requirements.dev.txt

@@ -5,3 +5,4 @@ mypy>=0.910
 pytest<=8.1.2
 pytest-cov
 pylint
+types-PyYAML


Oh where is this used? :o

Hmm I remember having an error saying types-PyYAML is not found, but I just retested it and it works fine now. Removed

deadlycoconuts · 2025-04-22T10:41:05Z

engines/pyfunc-ensembler-job/setup.py

+# get version from version.py
+spec = importlib.util.spec_from_file_location(
+    "pyfuncserver.version", os.path.join("version.py")
+)
+
+v_module = importlib.util.module_from_spec(spec)
+spec.loader.exec_module(v_module)
+
+version = v_module.VERSION


Whoa this looks complex 😅 Can't we retrieve the version in a simpler way like this https://github.com/caraml-dev/turing/blob/main/sdk/setup.py#L5? 🤔

bthari · 2025-04-17T09:22:39Z

.github/workflows/pyfunc-ensembler-job.yaml

        uses: actions/setup-python@v5
        with:
-          python-version: ${{ matrix.python-version }}
+          python-version: "3.10"


Is this intended to 3.10 and why are we still hard coded this specific version?

I need to put a python version here that we will use to do the testing, no specific reason on why I choose 3.10 tho haha. But it will be reverted to apply Zi's comment https://github.com/caraml-dev/turing/pull/404/files/69419b6ebed4ebbd1c8c1ab0167b30d2395fe352#r2053718016

bthari · 2025-04-17T09:27:36Z

api/turing/api/appcontext.go

@@ -94,6 +95,8 @@ func NewAppContext(
 	// Init ensemblers service
 	ensemblersService := service.NewEnsemblersService(db)

+	artifactService, err := initArtifactService(cfg)


Oh wait, it this expected that the error isn't being handled? 😮

Thanks for catching this 🤝, updated

… get artifact service type

Muhammad Naufal Andika Natsir Putra added 25 commits March 18, 2025 15:14

feat: adjust kaniko job flags

56bf4a0

feat: implement getHashedModelDependenciesUrl

b4035ef

feat: create unit test for getHashedModelDependenciesUrl

0a41d34

feat: refactor app.Dockerfile to use hashed dependency

538e74a

feat: call getHashedModelDependenciesUrl and pass to kanikojob

26a6ad3

chore: artifactService dependency injection

928a33c

chore: adjust imagebuilder_test

58e50f5

chore: delete unused attribute and reorder code

37b4d8e

chore: fix appcontext_test

0d64d89

feat: refactor ensembler job app.Dockerfile

b30d4ff

feat: remove python installation process on base Dockerfile, install …

76d4706

…python on the app.Dockerfile instead

feat: remove python installation from ensembling job base image

fa7c302

feat: update Makefile to publish turing pyfunc-ensembler-service to pypi

bf46cea

chore: add version when publishing pyfunc-ensembler-service package

ae204cf

chore: test app.Dockerfile changes

6bb664a

chore: try to create new conda env instead of updating the conda env …

f52d53b

…on app.Dockerfile

chore: change turing_dep value for testing

e74f95c

chore: change turing_dep value for testing

55b01af

feat: install turing-sdk via process_conda_env.sh

77cc5a1

chore: change TURING_DEP value

66b73c2

feat: use 1 base image for every python version

64dc081

chore: remove BaseImageRefTag

5135e3b

feat: create rule to build pyfunc-ensembler-job setup.py

b678b8d

feat: adjust app.Dockerfile and base Dockerfile for pyfunc-ensembler-job

b695c84

chore: add yq installation step on pyfunc-ensembler-job base Dockerfile

ca23625

naufalandika self-assigned this Apr 8, 2025

Muhammad Naufal Andika Natsir Putra added 2 commits April 8, 2025 11:20

chore: fix breaking appcontext_test.go

864a033

chore: refactor line with char > 120

65f9d3e

naufalandika force-pushed the DAT-2995_image_building_improvement branch from 00c1d89 to 65f9d3e Compare April 8, 2025 06:48

chore: remove unittest related to image tag

ddd1b85

naufalandika removed request for shydefoo, andreas-aji and vinoth-gojek April 14, 2025 06:17

naufalandika had a problem deploying to manual April 22, 2025 09:12 — with GitHub Actions Failure

naufalandika had a problem deploying to manual April 22, 2025 09:29 — with GitHub Actions Failure

deadlycoconuts approved these changes Apr 22, 2025

View reviewed changes

bthari reviewed Apr 25, 2025

View reviewed changes

feat: do pyfunc-ensembler-job unit testing for python 3.8 - 3.10

17712a3

naufalandika had a problem deploying to manual April 25, 2025 07:27 — with GitHub Actions Failure

feat: check error from initArtifactService

44f594a

naufalandika had a problem deploying to manual April 25, 2025 07:29 — with GitHub Actions Failure

naufalandika had a problem deploying to manual April 25, 2025 07:43 — with GitHub Actions Failure

chore: remove BaseImageRef and adjust the unittest

b520dd5

naufalandika had a problem deploying to manual April 25, 2025 07:56 — with GitHub Actions Failure

feat: use artifactService.GetType() instead of artifactServiceType to…

3df13eb

… get artifact service type

naufalandika had a problem deploying to manual April 25, 2025 08:06 — with GitHub Actions Failure

chore: reorder install yq command in Dockerfile

a0f11e6

naufalandika had a problem deploying to manual April 25, 2025 08:09 — with GitHub Actions Failure

naufalandika had a problem deploying to manual April 25, 2025 08:24 — with GitHub Actions Failure

feat: adjust how to get version from version.py

515ce0b

naufalandika had a problem deploying to manual April 25, 2025 08:45 — with GitHub Actions Error

feat: remove from requirements.txt

2ea96b3

naufalandika requested a deployment to manual April 25, 2025 08:58 — with GitHub Actions Waiting

naufalandika requested a deployment to manual April 25, 2025 09:13 — with GitHub Actions Waiting

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): image building process improvement #404

feat(api): image building process improvement #404

naufalandika commented Apr 8, 2025 •

edited

Loading

deadlycoconuts left a comment

deadlycoconuts Apr 22, 2025

naufalandika Apr 25, 2025

deadlycoconuts Apr 22, 2025

naufalandika Apr 25, 2025

deadlycoconuts Apr 22, 2025

naufalandika Apr 25, 2025

deadlycoconuts Apr 22, 2025

naufalandika Apr 25, 2025

deadlycoconuts Apr 22, 2025

naufalandika Apr 25, 2025

deadlycoconuts Apr 22, 2025

deadlycoconuts Apr 22, 2025

naufalandika Apr 25, 2025

deadlycoconuts Apr 22, 2025

naufalandika Apr 25, 2025

deadlycoconuts Apr 22, 2025

naufalandika Apr 25, 2025

deadlycoconuts Apr 22, 2025

naufalandika Apr 25, 2025

bthari Apr 17, 2025

naufalandika Apr 25, 2025

bthari Apr 17, 2025

naufalandika Apr 25, 2025

feat(api): image building process improvement #404

Are you sure you want to change the base?

feat(api): image building process improvement #404

Conversation

naufalandika commented Apr 8, 2025 • edited Loading

Context

imagebuilder

pyfunc-ensembler-service / pyfunc-ensembler-job package

sdk

Main Modifications

deadlycoconuts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naufalandika commented Apr 8, 2025 •

edited

Loading

`pyfunc-ensembler-service` / `pyfunc-ensembler-job` package