Skip to content
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

Simplify champion-challenger approach #7

Merged
merged 55 commits into from
May 9, 2023

Conversation

felix-datatonic
Copy link
Collaborator

@felix-datatonic felix-datatonic commented May 2, 2023

Description

Add Vertex AI services:

  • custom training job
  • model evaluation import
  • model versioning

Change of logic:

  • upload train script before triggering training pipeline
  • always upload a new model version after training
  • don't re-evaluate an existing champion, rather use historic evaluation results

How has this been tested?

  • make test-all-components
  • make e2e-tests

Checklist

  • I have commented my code, particularly in hard-to-understand areas
  • I have successfully run the E2E tests, and have included the links to the pipeline runs below
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated any relevant documentation to reflect my changes
  • I have assigned a reviewer and messaged them

Pipeline run links:

@felix-datatonic felix-datatonic changed the title Feature/simple cc approach Simplify champion-challenger approach May 2, 2023
@felix-datatonic felix-datatonic requested review from a user May 3, 2023 12:59
@felix-datatonic felix-datatonic self-assigned this May 3, 2023
@felix-datatonic felix-datatonic added the enhancement New feature or request label May 3, 2023
@felix-datatonic felix-datatonic marked this pull request as ready for review May 3, 2023 13:29
Pipfile Outdated Show resolved Hide resolved
Pipfile.lock Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
pipelines/pipelines/xgboost/training/assets/task.py Outdated Show resolved Hide resolved
requirements=requirements,
model_serving_container_image_uri=serving_container_uri,
)
cmd_args = [
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be less prescriptive with the command-line arguments. Ideally these would be provided as a list by the user as input to the component (but I see that is tricky given that it contains artifact paths). Could we output the paths as strings from previous components, construct the args in the pipeline definition and then pass into this components as args: List[str] ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately can't use op outputs (or pipeline params) in nested objects, leads to compilation error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested:

train_dataset_uri = (
    extract_bq_to_dataset(
        #...
    )
    .after(data_cleaning)
    .set_display_name("Extract train data to storage")
).outputs['dataset_gcs_uri']

train_args = [
    "--train_data", train_dataset_uri,
    # ...
]

train_model = custom_train_job(
    train_script_uri=train_script_uri,
    args=train_args,
    # ...
).set_display_name("Train model")

Error: TypeError: Object of type PipelineParam is not JSON serializable

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another option - pass in args as a string instead of List[str]? I think this would then work

eval_challenger = aip.model_evaluation.ModelEvaluation(challenger_evaluation)
metrics_champion = MessageToDict(eval_champion._gca_resource._pb)["metrics"]
metrics_challenger = MessageToDict(eval_challenger._gca_resource._pb)["metrics"]
metrics_challenger[eval_metric] -= 0.001 # TODO fake
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't forget this one!

@@ -26,7 +26,7 @@ steps:
mkdir -p ${COMMIT_SHA}/prediction/assets && \
cp -r pipelines/pipelines/${_PIPELINE_TEMPLATE}/training/assets ${COMMIT_SHA}/training/ && \
cp -r pipelines/pipelines/${_PIPELINE_TEMPLATE}/prediction/assets ${COMMIT_SHA}/prediction/ && \
gsutil cp -r ${COMMIT_SHA} ${_PIPELINE_PUBLISH_GCS_PATH}
gsutil cp -r ${COMMIT_SHA} ${_PIPELINE_PUBLISH_GCS_PATH}/${COMMIT_SHA}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felix-datatonic are you sure about this? Won't this create a nested directory?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, tested successfully. in the prior case only the contents of ${COMMIT_SHA} where copied to ${_PIPELINE_PUBLISH_GCS_PATH}. feel free to test it different combinations and values in Cloud Build though.

pipelines/pipelines/xgboost/training/pipeline.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants