[build] Add safety report to docker image #1186

tejaschumbalkar · 2021-06-25T19:36:27Z

Issue #, if available:

PR Checklist

I've prepended PR tag with frameworks/job this applies to : [mxnet, tensorflow, pytorch] | [ei/neuron] | [build] | [test] | [benchmark] | [ec2, ecs, eks, sagemaker]
(If applicable) I've documented below the DLC image/dockerfile this relates to
(If applicable) I've documented below the tests I've run on the DLC image
(If applicable) I've reviewed the licenses of updated and new binaries and their dependencies to make sure all licenses are on the Apache Software Foundation Third Party License Policy Category A or Category B license list. See https://www.apache.org/legal/resolved.html.
(If applicable) I've scanned the updated and new binaries to make sure they do not have vulnerabilities associated with them.

Pytest Marker Checklist

(If applicable) I have added the marker @pytest.mark.model("<model-type>") to the new tests which I have added, to specify the Deep Learning model that is used in the test (use "N/A" if the test doesn't use a model)
(If applicable) I have added the marker @pytest.mark.integration("<feature-being-tested>") to the new tests which I have added, to specify the feature that will be tested
(If applicable) I have added the marker @pytest.mark.multinode(<integer-num-nodes>) to the new tests which I have added, to specify the number of nodes used on a multi-node test
(If applicable) I have added the marker @pytest.mark.processor(<"cpu"/"gpu"/"eia"/"neuron">) to the new tests which I have added, if a test is specifically applicable to only one processor type

EIA/NEURON Checklist

When creating a PR:

I've modified src/config/build_config.py in my PR branch by setting ENABLE_EI_MODE = True or ENABLE_NEURON_MODE = True

When PR is reviewed and ready to be merged:

I've reverted the code change on the config file mentioned above

Benchmark Checklist

When creating a PR:

I've modified src/config/test_config.py in my PR branch by setting ENABLE_BENCHMARK_DEV_MODE = True

When PR is reviewed and ready to be merged:

I've reverted the code change on the config file mentioned above

Reviewer Checklist

For reviewer, before merging, please cross-check:

I've verified the code change on the config file mentioned above has already been reverted

Description:

This functionality will add the safety report as a part of the docker image.
Safety check will run on the image during build time and the safety report will be embedded into the image itself.
For any kind of image, it first builds the original image, then builds its common stage image and then pushes the common stage image to the repo. For eg., it first builds Standard images, then common stage images for Standard images and then pushes these common stage images to the repo. Thereafter it start processing the Example images and does the same for those as well.
During the sanity test, the tests will check if the safety report has been embedded or not and check if all the packages have passed the safety check.
In case any of the packages have failed the safety check, the sanity tests would fail.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

src/config/build_config.py

src/config/test_config.py

src/safety/safety_check_v2.py

test/dlc_tests/eks/mxnet/inference/test_eks_mxnet_inference.py

test/dlc_tests/sanity/test_safety_check.py

src/safety/safety_check_v2.py

src/safety/test_and_generate_python_safety_report.py

arjkesh · 2021-06-30T00:28:25Z

test/dlc_tests/sanity/test_safety_check.py

+            assert (safety_check.run_safety_check_with_ignore_list(docker_exec_cmd, ignore_str) == 0), \
+                f"Safety test failed for {image}"
+        else:
+            LOGGER.info(f"Safety check is complete as a part of docker build and report exist at {SAFETY_FILE}")


nit: typo - exists

test/dlc_tests/sanity/test_safety_check.py

buildspec.yml

tensorflow/training/docker/2.5/py3/Dockerfile.cpu

tensorflow/training/docker/2.5/py3/cu112/Dockerfile.gpu

src/image_builder.py

src/image.py

1. common_stage 2. constant.py (Only to this PR's code) 3. image_buider (Only to this PR's code) 4. image 5. test_safety_report_file 6. metrics.py (Only to this PR's code) 7. utils.py (Only to this PR's code) 8. safety_report_generato (Already ran) Not run on: 1. Dockerfile.common 2. Data Json 3. test_safety_check

…eport PULL request

…g name.

src/common_stage_image.py

saimidu · 2021-09-23T23:05:24Z

src/image.py

+        if self.to_push:
+            raise ValueError("Corresponding common stage image can only exist if the image is non-pushable")


Why can this only exist if the image is non-pushable?

Changed as per conversation.

tejaschumbalkar · 2021-09-24T18:01:51Z

data/ignore_ids_safety_scan.json

+                "38452":"for shipping pillow<=6.2.2 - the last available version for py2",
+                "35015":"for shipping pycrypto<=2.6.1 - the last available version for py2"
+            },
+            "py3": {


Should this contain the python minor version as well? What happens when a DLC has 2 python version support?

tejaschumbalkar · 2021-09-24T18:17:38Z

src/image_builder.py

-    THREADS = {}
+    # Standard images must be built before example images
+    # Example images will use standard images as base
+    # Common images must be built at the end as they will consume respective standard and example images


This needs to be reworded

tejaschumbalkar · 2021-09-24T18:20:57Z

src/image_builder.py

+    standard_images = [image for image in PRE_PUSH_STAGE_IMAGES if "example" not in image.name.lower()]
+    example_images = [image for image in PRE_PUSH_STAGE_IMAGES if "example" in image.name.lower()]
+    ALL_IMAGES = PRE_PUSH_STAGE_IMAGES + COMMON_STAGE_IMAGES
+    IMAGES_TO_PUSH = [image for image in ALL_IMAGES if image.to_push and image.to_build]


Can the IMAGES_TO_PUSH be COMMON_STAGE_IMAGES ?

saimidu

Approved.

…ed changes

tejaschumbalkar · 2021-09-25T02:50:14Z

src/image.py

-        LOGGER.info(f"Completed Push for {self.name}")
+        return self.build_status
+
+    def push_image_with_additional_tags(self):


Why do we need at separate function to push a additional tag? Why cant be this done in push_image

Seems that we need to tag the docker image first and push to ECR later.

tejaschumbalkar · 2021-09-25T02:52:15Z

src/image_builder.py

+    pushed and pushes them accordingly.
+
+    Note that the common stage images should always be built after the pre-push images of a
+    particular kind. This is because the Common stage images use are built on respctive 


nit: respctive spell

tejaschumbalkar · 2021-09-25T17:51:25Z

Changes looks good. The ECR images are tagged as expected. Let's comment the safety functionality as discussed.

tejaschumbalkar · 2021-09-25T17:51:39Z

Soft Approved!

tejaschumbalkar · 2021-09-26T00:59:25Z

PR is good to merge. Thanks for adding the functionality to our build system @shantanutrip !

tejaschumbalkar added 17 commits June 23, 2021 12:06

inital commit

ab8487a

Add safety report to docker image

d385f59

adapt build logic

e38004f

fix safety dir

e286853

address file strcuture

f3df287

add copy cmd

8400170

add bash option

821a4e2

fix file name

f610d45

add quotes

af82f00

install jq

ea87368

remove safety pacakge

e20b379

modify buildspec

67b969a

Merge branch 'master' into safety_report

6dd087c

add safety for TF2.5

7143bd8

support safety for TF2.5

2f23d6d

adapt the new safety workflow on the sanity test

f1f36c6

run sanity test

3ca89e5

tejaschumbalkar commented Jun 29, 2021

View reviewed changes

src/config/build_config.py Outdated Show resolved Hide resolved

src/config/test_config.py Outdated Show resolved Hide resolved

src/safety/safety_check_v2.py Outdated Show resolved Hide resolved

test/dlc_tests/eks/mxnet/inference/test_eks_mxnet_inference.py Outdated Show resolved Hide resolved

tejaschumbalkar added 2 commits June 29, 2021 12:27

run safety on PR

7603d30

revert unrelated change

d887fb1

tejaschumbalkar commented Jun 29, 2021

View reviewed changes

test/dlc_tests/sanity/test_safety_check.py Outdated Show resolved Hide resolved

revert temp changes

8d6982e

arjkesh reviewed Jun 30, 2021

View reviewed changes

buildspec.yml Outdated Show resolved Hide resolved

jeet4320 reviewed Jul 2, 2021

View reviewed changes

tensorflow/training/docker/2.5/py3/Dockerfile.cpu Outdated Show resolved Hide resolved

jeet4320 reviewed Jul 2, 2021

View reviewed changes

tensorflow/training/docker/2.5/py3/cu112/Dockerfile.gpu Outdated Show resolved Hide resolved

tejaschumbalkar added 2 commits July 27, 2021 15:51

multi stage docker build

cb6c347

Merge branch 'master' into safety_report

36fb18f

shantanutrip reviewed Aug 11, 2021

View reviewed changes

src/image_builder.py Outdated Show resolved Hide resolved

shantanutrip reviewed Aug 12, 2021

View reviewed changes

src/image.py Show resolved Hide resolved

shantanutrip added 10 commits September 5, 2021 21:50

Added logging method to image

699caca

Merge remote-tracking branch 'alt/master'(AWS-DLC repo) into safety-r…

9b6ea43

…eport PULL request

Minor changed

664e946

Add multistage.common tag

0a0aeeb

Merging from alt master after DIY changes

d9d09b8

Added the process_image function to build and push images. Changed ta…

4bf75d9

…g name.

Made the get_safety_ignore_dict method bit generic

742d1c9

Merge remote-tracking branch 'alt/master' into safety-report-tejas

2b3d735

Removed the if condition for context

dd1c08c

saimidu previously approved these changes Sep 23, 2021

View reviewed changes

Addressed the nit changes

cae71c0

shantanutrip dismissed saimidu’s stale review via cae71c0 September 24, 2021 02:35

shantanutrip added 2 commits September 24, 2021 02:37

Merge remote-tracking branch 'alt/master' into safety-report-tejas

76335c1

Ignoring TF2.6 vulnerability

0f7416c

tejaschumbalkar commented Sep 24, 2021

View reviewed changes

saimidu reviewed Sep 24, 2021

View reviewed changes

shantanutrip added 3 commits September 24, 2021 22:19

Added the multi-tagging mechanism

feed6bd

Configured test env file to dump no appended tags. Other loggin relat…

9c192c9

…ed changes

Merge remote-tracking branch 'alt/master' into safety-report-tejas

bde69df

tejaschumbalkar commented Sep 25, 2021

View reviewed changes

shantanutrip added 2 commits September 25, 2021 23:38

Disabling Common Stage

0996ac0

Fix

e90439e

shantanutrip approved these changes Sep 26, 2021

View reviewed changes

shantanutrip merged commit 954f91e into aws:master Sep 26, 2021

arjkesh mentioned this pull request Oct 2, 2021

Fix build logic from safety check PR #1396

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[build] Add safety report to docker image #1186

[build] Add safety report to docker image #1186

tejaschumbalkar commented Jun 25, 2021 •

edited by shantanutrip

Loading

arjkesh Jun 30, 2021

saimidu Sep 23, 2021

shantanutrip Sep 24, 2021 •

edited

Loading

tejaschumbalkar Sep 24, 2021

shantanutrip Sep 25, 2021

tejaschumbalkar Sep 24, 2021

shantanutrip Sep 25, 2021

tejaschumbalkar Sep 24, 2021

shantanutrip Sep 25, 2021

saimidu left a comment •

edited

Loading

tejaschumbalkar Sep 25, 2021 •

edited

Loading

tejaschumbalkar Sep 25, 2021

tejaschumbalkar Sep 25, 2021

tejaschumbalkar commented Sep 25, 2021

tejaschumbalkar commented Sep 25, 2021

tejaschumbalkar commented Sep 26, 2021

		if self.to_push:
		raise ValueError("Corresponding common stage image can only exist if the image is non-pushable")

[build] Add safety report to docker image #1186

[build] Add safety report to docker image #1186

Conversation

tejaschumbalkar commented Jun 25, 2021 • edited by shantanutrip Loading

PR Checklist

Pytest Marker Checklist

EIA/NEURON Checklist

Benchmark Checklist

Reviewer Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shantanutrip Sep 24, 2021 • edited Loading

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

saimidu left a comment • edited Loading

Choose a reason for hiding this comment

tejaschumbalkar Sep 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejaschumbalkar commented Sep 25, 2021

tejaschumbalkar commented Sep 25, 2021

tejaschumbalkar commented Sep 26, 2021

tejaschumbalkar commented Jun 25, 2021 •

edited by shantanutrip

Loading

shantanutrip Sep 24, 2021 •

edited

Loading

saimidu left a comment •

edited

Loading

tejaschumbalkar Sep 25, 2021 •

edited

Loading