-
Notifications
You must be signed in to change notification settings - Fork 24
feat(api): image building process improvement #404
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
56bf4a0
b4035ef
0a41d34
538e74a
26a6ad3
928a33c
58e50f5
37b4d8e
0d64d89
b30d4ff
76d4706
fa7c302
bf46cea
ae204cf
6bb664a
f52d53b
e74f95c
55b01af
77cc5a1
66b73c2
64dc081
5135e3b
b678b8d
b695c84
ca23625
864a033
65f9d3e
ddd1b85
2d620f7
7e0ba5f
7480267
56ec882
d1cc242
93dae7e
438f646
f1b96e7
671b7e8
60fdba1
99ba890
e971d3c
b5cc22d
2c0c613
6d580cc
fbcedf1
19d42d0
4dabb22
690b589
74fcb5a
4b66da3
8cd9f07
db6bcad
557c0a3
f50fad9
703bffd
65bfde2
6c2d3b1
92702c2
da5f434
a9c1b97
50dc701
daeb4b5
f62ccea
c590d4b
953ec56
166b0b1
5ac74dc
6a81433
acb00b6
6480f68
3649ed9
69419b6
17712a3
44f594a
b520dd5
3df13eb
a0f11e6
515ce0b
2ea96b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,37 +1,19 @@ | ||
name: engines/pyfunc-ensembler-job | ||
|
||
on: | ||
# Automatically run CI on Release and Pre-Release tags and main branch | ||
# (only if there are changes to relevant paths) | ||
push: | ||
tags: | ||
- "pyfunc-ensembler-job/v[0-9]+.[0-9]+.[0-9]+*" | ||
branches: | ||
- main | ||
paths: | ||
- ".github/workflows/pyfunc-ensembler-job.yaml" | ||
- "engines/pyfunc-ensembler-job/**" | ||
- "sdk/**" | ||
|
||
# Automatically run CI on branches, that have active PR opened | ||
pull_request: | ||
branches: | ||
- main | ||
paths: | ||
- ".github/workflows/pyfunc-ensembler-job.yaml" | ||
- "engines/pyfunc-ensembler-job/**" | ||
- "sdk/**" | ||
|
||
# To make it possible to trigger e2e CI workflow for any arbitrary git ref | ||
workflow_dispatch: | ||
# The package build by this job requires latest version of sdk, | ||
# so we can only run this workflow after sdk workflow is successfully run | ||
workflow_run: | ||
workflows: ["sdk"] | ||
types: | ||
- completed | ||
|
||
jobs: | ||
test: | ||
runs-on: ubuntu-latest | ||
strategy: | ||
matrix: | ||
python-version: ["3.8", "3.9", "3.10"] | ||
|
||
steps: | ||
- uses: actions/checkout@v4 | ||
|
||
|
@@ -40,7 +22,6 @@ jobs: | |
with: | ||
python-version: ${{ matrix.python-version }} | ||
cache-dependency-path: | | ||
engines/pyfunc-ensembler-job/env-${{ matrix.python-version }}.yaml | ||
engines/pyfunc-ensembler-job/requirements.txt | ||
engines/pyfunc-ensembler-job/requirements.dev.txt | ||
|
||
|
@@ -70,7 +51,7 @@ jobs: | |
- id: release-rules | ||
uses: ./.github/actions/release-rules | ||
with: | ||
prefix: pyfunc-ensembler-job/ | ||
prefix: python/ | ||
|
||
publish: | ||
# Automatically publish release and pre-release artifacts. | ||
|
@@ -81,14 +62,12 @@ jobs: | |
# Dev build can be released either from the 'main' branch or | ||
# by running this workflow manually with `workflow_dispatch` event. | ||
if: >- | ||
contains('release,pre-release', needs.release-rules.outputs.release-type) | ||
(contains('release,pre-release', needs.release-rules.outputs.release-type) | ||
|| ( 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' }} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand correctly, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
the |
||
environment: ${{ needs.release-rules.outputs.release-type == 'dev' && 'manual' || '' }} | ||
runs-on: ubuntu-latest | ||
strategy: | ||
matrix: | ||
python-version: ["3.8", "3.9", "3.10"] | ||
needs: | ||
- release-rules | ||
- test | ||
|
@@ -109,11 +88,20 @@ jobs: | |
working-directory: engines/pyfunc-ensembler-job | ||
env: | ||
DOCKER_REGISTRY: ghcr.io/${{ github.repository }} | ||
PYTHON_VERSION: ${{ matrix.python-version }} | ||
run: | | ||
set -o pipefail | ||
make build-image | tee output.log | ||
echo "::set-output name=pyfunc-ensembler-job::$(sed -n 's%Building docker image: \(.*\)%\1%p' output.log)" | ||
|
||
- name: Publish Pyfunc Ensembler Job Docker Image | ||
run: docker push ${{ steps.build.outputs.pyfunc-ensembler-job }} | ||
|
||
- name: Publish pyfunc-ensembler-job package | ||
env: | ||
TWINE_USERNAME: ${{ secrets.PYPI_USERNAME }} | ||
TWINE_PASSWORD: ${{ secrets.PYPI_API_TOKEN }} | ||
working-directory: engines/pyfunc-ensembler-job | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ooh, why do we need to store this as a GitHub output? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't remember my reason on doing this haha, maybe because the |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ import ( | |
|
||
mlpcluster "github.com/caraml-dev/mlp/api/pkg/cluster" | ||
|
||
"github.com/caraml-dev/mlp/api/pkg/artifact" | ||
batchensembling "github.com/caraml-dev/turing/api/turing/batch/ensembling" | ||
batchrunner "github.com/caraml-dev/turing/api/turing/batch/runner" | ||
"github.com/caraml-dev/turing/api/turing/cluster" | ||
|
@@ -94,6 +95,11 @@ func NewAppContext( | |
// Init ensemblers service | ||
ensemblersService := service.NewEnsemblersService(db) | ||
|
||
artifactService, err := initArtifactService(cfg) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh wait, it this expected that the error isn't being handled? 😮 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for catching this 🤝, updated |
||
if err != nil { | ||
return nil, errors.Wrapf(err, "Failed initializing artifact service") | ||
} | ||
|
||
if cfg.BatchEnsemblingConfig.Enabled { | ||
if cfg.BatchEnsemblingConfig.JobConfig == nil { | ||
return nil, errors.Wrapf(err, "BatchEnsemblingConfig.JobConfig was not set") | ||
|
@@ -124,7 +130,7 @@ func NewAppContext( | |
ensemblingImageBuilder, err = imagebuilder.NewEnsemblerJobImageBuilder( | ||
imageBuildingController, | ||
*cfg.BatchEnsemblingConfig.ImageBuildingConfig, | ||
cfg.MlflowConfig.ArtifactServiceType, | ||
artifactService, | ||
) | ||
if err != nil { | ||
return nil, errors.Wrapf(err, "Failed initializing ensembling image builder") | ||
|
@@ -159,7 +165,7 @@ func NewAppContext( | |
ensemblerServiceImageBuilder, err := imagebuilder.NewEnsemblerServiceImageBuilder( | ||
clusterControllers[cfg.EnsemblerServiceBuilderConfig.ClusterName], | ||
*cfg.EnsemblerServiceBuilderConfig.ImageBuildingConfig, | ||
cfg.MlflowConfig.ArtifactServiceType, | ||
artifactService, | ||
) | ||
if err != nil { | ||
return nil, errors.Wrapf(err, "Failed initializing ensembler service builder") | ||
|
@@ -236,3 +242,16 @@ func buildKubeconfigStore(mlpSvc service.MLPService, cfg *config.Config) (map[st | |
} | ||
return k8sConfigStore, nil | ||
} | ||
|
||
func initArtifactService(cfg *config.Config) (artifact.Service, error) { | ||
if cfg.MlflowConfig.ArtifactServiceType == "gcs" { | ||
return artifact.NewGcsArtifactClient() | ||
} | ||
if cfg.MlflowConfig.ArtifactServiceType == "s3" { | ||
return artifact.NewS3ArtifactClient() | ||
} | ||
if cfg.MlflowConfig.ArtifactServiceType == "nop" { | ||
return artifact.NewNopArtifactClient(), nil | ||
} | ||
return nil, fmt.Errorf("invalid artifact service type %s", cfg.MlflowConfig.ArtifactServiceType) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -169,10 +169,6 @@ type ImageBuildingConfig struct { | |
BuildTimeoutDuration time.Duration `validate:"required"` | ||
// DestinationRegistry is the registry of the newly built ensembler image. | ||
DestinationRegistry string `validate:"required"` | ||
// BaseImageRef is the image name of the base ensembler image built from the | ||
// engines/pyfunc-ensembler-*/Dockerfile. It's a map of image names, per | ||
// minor python version supported by the SDK. | ||
BaseImageRef map[string]string `validate:"required"` | ||
// KanikoConfig contains the configuration related to the kaniko executor image builder. | ||
KanikoConfig KanikoConfig `validate:"required"` | ||
// TolerationName allow the scheduler to schedule image building jobs with the matching name | ||
|
@@ -181,6 +177,8 @@ type ImageBuildingConfig struct { | |
NodeSelector map[string]string | ||
// Value for cluster-autoscaler.kubernetes.io/safe-to-evict annotation | ||
SafeToEvict bool | ||
// BaseImageRef is the image name of the base ensembler image built from the engines/pyfunc-ensembler-*/Dockerfile | ||
BaseImage string | ||
Comment on lines
+180
to
+181
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can now remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. correct, thanks for catching this. Updated |
||
} | ||
|
||
// Resource contains the Kubernetes resource request and limits | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow this is cool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I can't test this until the workflow is merged to main tho, hopefully it runs as expected