Separate refactoring from #12276 in a prior PR #12296

larroy · 2018-08-22T21:14:49Z

Description

This is a prior refactor separated from #12276 at request from the reviewer.

Fixes --no-dockerhub-cache options and docker's --cache-from so local cache can be used on request by providing --no-dockerhub-cache.

Checked with mypy and pycharm code analyze.

@KellenSunderland

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 http://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

Comments

If this change is a backward incompatible change, why must this change be made.
Interesting edge cases to note here

lebeg

Good change

lebeg · 2018-08-23T08:26:23Z

ci/build.py

+        each retry
+    :type backoff: int
+    """
+    import time


I think it's better to do the imports at the top of the file in this case

Why is it better? it's perfectly legal to do this, and some consider best practice to scope imports to the minimum scope necessary, like for example function scope.

But would it be imported every time the retry function gets called?

https://stackoverflow.com/questions/3095071/in-python-what-happens-when-you-import-inside-of-a-function

I don't feel strongly about this in either direction, for some functions like this is better so it's self contained. I can also move this function to util.

Ok, I see. Thanks!

lebeg · 2018-08-23T08:31:05Z

ci/build.py

            if args.build_only:
                continue
-            build_platform = "build_{}".format(platform)
-            cmd = ["/work/mxnet/ci/docker/runtime_functions.sh", build_platform]
+            git_cleanup()


This is good, but maybe we should issue a warning here with confirmation.

Actually, how this would work when all platforms are build one after another? Only the latest artifacts survive?

How would this work if for example I want to test a new file locally that hasn't yet been added to git?

lebeg · 2018-08-23T08:32:29Z

ci/build.py

+            build_platform = "build_{}".format(platform)
+            plat_buildir = os.path.abspath(os.path.join(get_mxnet_root(), '..',
+                                                        "mxnet_{}".format(build_platform)))
+            if os.path.exists(plat_buildir):


How would the path exist if git_cleanup was called before?

This is addressed, I removed git cleanup.

lebeg · 2018-08-23T08:34:05Z

ci/build.py

@@ -383,7 +472,8 @@ def use_cache():

 ./build.py -a

-    Builds for all platforms and leaves artifacts in build_<platform>
+    Builds for all platforms and leaves artifacts in build_<platform>. **WARNING** it performs git


Maybe a -y for quiet confirmation

removed that git cleanup.

KellenSunderland · 2018-08-23T07:41:41Z

ci/build.py

@@ -284,7 +364,7 @@ def script_name() -> str:
                        default=1,
                        type=int)

-    parser.add_argument("-c", "--cache", action="store_true",
+    parser.add_argument("--no-cache", action="store_true",


What's the background for this change? I'm a little concerned that changing script args could have downstream impact so.

Yes, I agree that this is changing behavior of the script and does not fall under "minor refactorings". Even if it's a oneliner - it needs a separate PR.

I will split it again then. But this doesn't affect CI given the command line used in the Jenkinsfile.

It's too much trouble to split, as agreed in 1:1 I improved documentation about this flag.

KellenSunderland · 2018-08-23T08:49:31Z

ci/build.py

            if args.build_only:
                continue
-            build_platform = "build_{}".format(platform)
-            cmd = ["/work/mxnet/ci/docker/runtime_functions.sh", build_platform]
+            git_cleanup()


How would this work if for example I want to test a new file locally that hasn't yet been added to git?

lebeg · 2018-08-23T09:39:13Z

ci/build.py


    command = list(chain(*args.command))
    docker_binary = get_docker_binary(args.nvidiadocker)
    shared_memory_size = args.shared_memory_size
    num_docker_build_retires = args.docker_build_retries

    if args.list:
-        list_platforms()
+        print(list_platforms())


You used logger before, maybe use it here as well instead of print?

Is different concerns, this prints unconditionally to stdout which is what you want. Logging can be supressed. This is intentional

lebeg · 2018-08-23T09:42:15Z

ci/build.py

@@ -284,7 +364,7 @@ def script_name() -> str:
                        default=1,
                        type=int)

-    parser.add_argument("-c", "--cache", action="store_true",
+    parser.add_argument("--no-cache", action="store_true",


Yes, I agree that this is changing behavior of the script and does not fall under "minor refactorings". Even if it's a oneliner - it needs a separate PR.

lebeg · 2018-08-23T13:40:10Z

ci/build.py

@@ -284,8 +288,9 @@ def script_name() -> str:
                        default=1,
                        type=int)

-    parser.add_argument("-c", "--cache", action="store_true",
-                        help="Enable docker registry cache")
+    parser.add_argument("--no-cache", action="store_true",


I find the name a bit confusing, maybe --dockerhub--cache?

lebeg · 2018-08-23T13:59:18Z

ci/build.py

@@ -284,8 +288,10 @@ def script_name() -> str:
                        default=1,
                        type=int)

-    parser.add_argument("-c", "--cache", action="store_true",
-                        help="Enable docker registry cache")
+    parser.add_argument("-c", "--no-dockerhub-cache", action="store_true",


Isn't this the same as not providing --docker-registry?

I would rather have it explicit. but we would have to code it if we want it the way you say.

The problem is that for "default" behaviour, which I assume is use local cache (as for normal docker builds) we need to provide --no-dockerhub-cache explicitly. I think it should be the other way around.

-c is not a good shortening anymore for the option

The default behavior remains unchanged if I'm not mistaken. Which is to use the dockerhub cache.

Kellen. One doesn't need to specify the registry. Hence this option to disable cache which is enabled by default. I'm not sure what's the cause of confusion here.

This seems like a work around for broken behaviour to me. Would you mind if we leave it out of this PR, and try to think about how to fix the root cause of the problem? To me it makes sense to not use remote caching if a user doesn't specify '--docker-registry'.

That’s bad because we want to use cache by default. This was discussed at length with Marco before and we came to agree on this behavior. I wouldn’t want to change that.

Ok, that's the information I was missing here. In this case it makes sense to me.

lebeg · 2018-08-23T14:00:07Z

ci/build.py

+    if instance_id:
+        logging.info("EC2 Instance id: %s", instance_id)
+    pp = pprint.PrettyPrinter(indent=4)
+    logging.debug("Build environment: %s", pp.pformat(dict(os.environ)))


Should this be info? The global level is at that level. Or should we have separate levels for ci and local?

I think it's too verbose. In general I don't think we need to view this environment. I had it to diagnose. I wouldn't bother right now.

lebeg · 2018-08-23T14:02:02Z

ci/build.py

-    logging.getLogger().setLevel(logging.INFO)
+def log_environment():
+    instance_id = ec2_instance_id_hostname()
+    if instance_id:


Is this always true? The function returns '?' if something goes wrong

Empty string is false. In that case is not printed. For example outside of ci or locally.

lebeg · 2018-08-23T14:02:53Z

ci/util.py

+                try:
+                    return f(*args, **kwargs)
+                except target_exception as e:
+                    logging.warning("Exception: %s, Retrying in %d seconds...", str(e), mdelay)


Should we propagate the exception if we out of retries?

Last call to f is propagating the exception as you say.

Yes, you're right

KellenSunderland

LGTM modulo the new caching arguments, which I'm not still 100% understanding.

KellenSunderland · 2018-08-23T16:41:08Z

ci/build.py

+           "-f", get_dockerfile(platform),
+           "--build-arg", "USER_ID={}".format(os.getuid()),
+           "--build-arg", "GROUP_ID={}".format(os.getgid())]
+    if use_cache:


Nit: I find this a bit of a misnomer. It sounds like we're disabling docker-cache, which is possible, but it's not what's done here. Reading the code I would expect it to pass --no-cache to the docker cmd being run. Maybe a more specific name could help?

I'm open. Any suggestions?

KellenSunderland · 2018-08-23T16:43:08Z

ci/build.py

+        cmd.extend(["--cache-from", tag])
+    cmd.extend(["-t", tag, get_dockerfiles_path()])
+
+    @retry(subprocess.CalledProcessError, tries=num_retries)


Really like this.

KellenSunderland · 2018-08-23T16:43:58Z

ci/build.py

    if not dry_run and interactive:
        into_cmd = deepcopy(runlist)
        # -ti can't be after the tag, as is interpreted as a command so hook it up after the -u argument
        idx = into_cmd.index('-u') + 2
        into_cmd[idx:idx] = ['-ti']
-        cmd = '\\\n\t'.join(into_cmd)
+        cmd = ' \\\n\t'.join(into_cmd)


Thanks for fixing this.

sandeep-krishnamurthy · 2018-08-23T20:30:08Z

@lebeg - This PR is ready to go, if your concerns are addressed.

lebeg

Sure, lgtm

sandeep-krishnamurthy · 2018-08-24T16:49:14Z

@marcoabreu - can you take a look at this?

marcoabreu · 2018-08-27T12:59:56Z

LGTM, thanks for iterating on this one!

* Separate minor refactoring from apache#12276 in a prior PR * Address CR comments

lebeg reviewed Aug 23, 2018

View reviewed changes

KellenSunderland reviewed Aug 23, 2018

View reviewed changes

lebeg reviewed Aug 23, 2018

View reviewed changes

larroy force-pushed the build_py branch from d2faf77 to b75809d Compare August 23, 2018 13:16

Separate minor refactoring from apache#12276 in a prior PR

9fadeb4

larroy force-pushed the build_py branch 2 times, most recently from c513740 to bc5d32b Compare August 23, 2018 13:19

larroy changed the title ~~Separate minor refactoring from #12276 in a prior PR~~ Separate refactoring from #12276 in a prior PR Aug 23, 2018

larroy force-pushed the build_py branch 2 times, most recently from dc6b876 to 2cea66c Compare August 23, 2018 13:37

lebeg reviewed Aug 23, 2018

View reviewed changes

larroy force-pushed the build_py branch 2 times, most recently from 3b6e8aa to 04b8cd8 Compare August 23, 2018 13:42

Address CR comments

fdb4f47

larroy force-pushed the build_py branch from 04b8cd8 to fdb4f47 Compare August 23, 2018 13:45

lebeg reviewed Aug 23, 2018

View reviewed changes

larroy mentioned this pull request Aug 23, 2018

[Don't merge][Review] A solution to prevent zombie containers locally and in CI #12276

Closed

5 tasks

KellenSunderland suggested changes Aug 23, 2018

View reviewed changes

KellenSunderland approved these changes Aug 23, 2018

View reviewed changes

lebeg approved these changes Aug 24, 2018

View reviewed changes

sandeep-krishnamurthy added Build pr-awaiting-review PR is waiting for code review labels Aug 24, 2018

marcoabreu merged commit 6a7bfe9 into apache:master Aug 27, 2018

anirudh2290 pushed a commit to anirudh2290/mxnet that referenced this pull request Sep 19, 2018

Separate refactoring from apache#12276 in a prior PR (apache#12296)

f44fc79

* Separate minor refactoring from apache#12276 in a prior PR * Address CR comments

Separate refactoring from #12276 in a prior PR #12296

Separate refactoring from #12276 in a prior PR #12296

Conversation

larroy commented Aug 22, 2018 • edited Loading

Description

Checklist

Essentials

Comments

lebeg left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larroy Aug 23, 2018 • 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

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

larroy Aug 23, 2018 • 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

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

KellenSunderland left a 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

sandeep-krishnamurthy commented Aug 23, 2018

lebeg left a comment

Choose a reason for hiding this comment

sandeep-krishnamurthy commented Aug 24, 2018

marcoabreu commented Aug 27, 2018

larroy commented Aug 22, 2018 •

edited

Loading

larroy Aug 23, 2018 •

edited

Loading

larroy Aug 23, 2018 •

edited

Loading