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

Simplified build with Azure CLI; various bugfixes #121

Merged
merged 6 commits into from
Dec 6, 2019

Conversation

algattik
Copy link
Contributor

@algattik algattik commented Nov 28, 2019

bump pip requirements versions (microsoft#104)
* Use Azure CLI tasks to remove need for variables SP_APP_ID, SP_APP_SECRET, SUBSCRIPTION_ID, TENANT_ID
* Updated gettin_started.md to point out variable path to artifact (microsoft#117)
* Made random hyperparameter (ridge regression alpha) a pipeline parameter, setting its value in the DevOps pipeline (microsoft#107)
* Changed unit test to test actual training code (partially solves microsoft#74)
* Fixed: attach_compute should set exit code after exception (microsoft#113)
* Fixed scoring endpoint HTTP behavior (microsoft#110)
* Fixed PipelineParameters format in call to Azure ML Extension (microsoft#118)
* Fixed Model build environment for Azure Web App for containers (microsoft#119)
algattik added a commit to algattik/MLOpsPython that referenced this pull request Dec 2, 2019
Builds on top of PR microsoft#121, and:
- Replaces separate build + release pipelines with a single multi-stage pipeline with stages for Build, Train, ACI, AKS and Webapp deployment.
- ACI, AKS and Webapp deployment are optionally enabled (through Variables).
- Added example of custom traces (logging) and correlating traces with requests with Kusto queries.
- Added example of distributed tracing (client passes a trace header, logged by the scoring service).

![image](https://user-images.githubusercontent.com/22099722/69975882-52306580-1528-11ea-9705-65dc2ab737ea.png)
@dtzar dtzar self-assigned this Dec 4, 2019
Copy link
Member

@dtzar dtzar left a comment

Choose a reason for hiding this comment

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

Overall looks good! Can you also please modify the description text so that the issues will automatically close after PR merge?

.pipelines/azdo-base-pipeline.yml Outdated Show resolved Hide resolved
.pipelines/azdo-ci-build-train.yml Outdated Show resolved Hide resolved

![release_webapp](./images/release-webapp-pipeline.PNG)

In the Variables tab, link the pipeline to your variable group (`devopsforai-aml-vg`). In the variable group definition, add the following variables:
Copy link
Member

Choose a reason for hiding this comment

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

These should already be picked up by the variables template file. If you want to add instructions for people to modify that file, then no problem else remove this section.

Copy link
Contributor Author

@algattik algattik Dec 5, 2019

Choose a reason for hiding this comment

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

This is the doc for the release pipeline, which if I'm not mistaken the user must create by hand from scratch. (This is removed in PR #122 that replace the release pipeline with a multi-stage build pipeline)

ml_service/pipelines/run_train_pipeline.py Show resolved Hide resolved
set -euo pipefail # strict mode, fail on error
set -x # verbose

docker run \
Copy link
Member

Choose a reason for hiding this comment

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

Eugene already has a pipeline file to do this and not the best idea to do docker-in-docker. I'd prefer you remove this file and we can work into a different PR.

Copy link
Contributor Author

@algattik algattik Dec 5, 2019

Choose a reason for hiding this comment

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

This is used in the release pipeline (created by hand) which per getting_started.md runs directly on the Ubuntu 16 build agent, so there is no use of docker-in-docker.

I agree this script is pretty ugly-looking. It is removed anyway with PR #122, which does away with release pipelines.

If I just remove the file, the getting_started file will not have complete instructions to release a webapp...

tests/requirements.txt Outdated Show resolved Hide resolved
@algattik
Copy link
Contributor Author

algattik commented Dec 6, 2019

Thanks a lot for the detailed review! Comments have been addressed with code updates or answers above.

Because of changes to requirements.txt, running the build from the PR now requires the image at mcr.microsoft.com/mlops/python:latest to have been updated accordingly. This is why the unit tests are now failing on the PR check. I've verified a modified version of the PR where the pipeline runs on a container deployed in a private registry, built from environment_setup/Dockerfile.

@algattik algattik requested a review from dtzar December 6, 2019 04:47
algattik added a commit to algattik/MLOpsPython that referenced this pull request Dec 6, 2019
@dtzar dtzar merged commit 11716e8 into microsoft:master Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants