Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[CD] Adds python docker pipeline #16547

Merged
merged 10 commits into from
Oct 21, 2019
Merged

[CD] Adds python docker pipeline #16547

merged 10 commits into from
Oct 21, 2019

Conversation

perdasilva
Copy link
Contributor

@perdasilva perdasilva commented Oct 19, 2019 •
edited
Loading

Description

Updates CD pipeline to post python docker images. Addressed comments from closed PR #16435
@marcoabreu I've decided I would like to finish this. I've addressed your previous comments.

Example CI-dev run

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

marcoabreu
marcoabreu approved these changes Oct 19, 2019
View reviewed changes
Copy link
Contributor

@marcoabreu marcoabreu left a comment

Choose a reason for hiding this comment

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

This is awesome!! Thanks a ton. Ready to merge whenever you are ready

aaronmarkham
aaronmarkham reviewed Oct 19, 2019
View reviewed changes
Copy link
Contributor

@aaronmarkham aaronmarkham left a comment

Choose a reason for hiding this comment

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

I just have a few questions in the review.

I looked at the shell output from the push stage and was hoping to see a link to the asset. Where'd they go? Seems like the only references are abstracted to whatever the secrets config is...

This is great btw.

cd/utils/mxnet_base_image.sh Outdated
cu90*)
echo "nvidia/cuda:9.0-cudnn7-runtime-ubuntu16.04"
;;
cu91*)
Copy link
Contributor

Choose a reason for hiding this comment

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

I always pick on this one... but isn't it not supported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I'll remove

ci/build.py
import pprint


class Cleanup:
Copy link
Contributor

Choose a reason for hiding this comment

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

So all of this is in the new docker client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup - it all moved to the "safe_docker_run.py" script

ci/build.py
@@ -305,61 +246,7 @@ def container_run(platform: str,
{'bind': '/work/ccache', 'mode': 'rw'},
},
environment=environment)
try:
logging.info("Started container: %s", trim_container_id(container.id))
# Race condition:
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is no longer an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's still a thing - the signal blocking has moved to the "safe_docker_run.py" script ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll move these comments in there for clarity though - good catch!

@perdasilva
Copy link
Contributor Author

@aaronmarkham you can see the push statement - it's not possible to put a link to the image (the best that can be done is a link to the repository. I'll add a message with the docker pull command to run to get it. Let me know if you would prefer something else =)

@aaronmarkham
Copy link
Contributor

Just notice this:
"SecretString":"{\\"username\\": \\"mxnetcddev\\", \\"password\\": \\"uuz:C^P;2lOR3Z:mNi|:8A6^}UUKtq9F\\"}"
And the security token is output in the log too... maybe they're one time use? Or?...

@perdasilva
Copy link
Contributor Author

@aaronmarkham nope, that's bad. I've rotated the credentials. I've checked the DockerHub account and nothing has changed there. Thanks for catching this! I've updated the logging configuration and I'm running a job to double check.

@perdasilva
Copy link
Contributor Author

Okay, looks good now. I'm.glad you took the time to have a look! Thank you!!

@perdasilva
Copy link
Contributor Author

@aaronmarkham if you are cool with everything, please merge when you get a chance =D

@marcoabreu marcoabreu merged commit 91bb398 into apache:master Oct 21, 2019
@marcoabreu
Copy link
Contributor

Again, thanks a lot for this contribution! Can you let us (and dev@) know once the images are being published?

@leezu leezu mentioned this pull request Mar 13, 2020
Open
@leezu leezu mentioned this pull request Jun 3, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@aaronmarkham aaronmarkham aaronmarkham left review comments

@marcoabreu marcoabreu marcoabreu approved these changes

@szha szha Awaiting requested review from szha szha is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@perdasilva @aaronmarkham @marcoabreu