[MXNET-769] Usability improvements to windows builds #11947

larroy · 2018-07-31T15:53:36Z

Description

This PR makes it a single command to build MXNet in windows.
Adjusts CI to use the windows build script
Greatly simplifies the windows build and test logic, making it contained on the MXNet repository and able to reproduce locally with ease.

#8714
#11100
#10166
#10049

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

marcoabreu · 2018-07-31T20:59:31Z

Awesome!

larroy · 2018-08-01T14:55:12Z

@Chancebair

larroy · 2018-08-01T14:55:32Z

@KellenSunderland @lebeg

Adjust Jenkins builds to use ci/build_windows.py Issues: apache#8714 apache#11100 apache#10166 apache#10049

KellenSunderland

Couple nits, looks good so far.

KellenSunderland · 2018-08-02T20:54:32Z

ci/build.py

@@ -39,6 +39,7 @@
 from itertools import chain
 from subprocess import call, check_call
 from typing import *
+from util import *


Is this addition needed? Looks like you only removed from this file.

it's needed for get_mxnet_root

KellenSunderland · 2018-08-02T20:54:59Z

ci/build.py

 def buildir() -> str:
    return os.path.join(get_mxnet_root(), "build")

-


Should be two spaces between defs.

KellenSunderland · 2018-08-02T20:56:13Z

ci/build_windows.py

+        check_call("ninja", shell=True)
+
+def main():
+    logging.getLogger().setLevel(logging.INFO)


Don't need this line, info is default.

@KellenSunderland Python disagrees:

pllarroy@186590d670bd:0:~$ python Python 2.7.15 (default, May 1 2018, 16:44:37) [GCC 4.2.1 Compatible Apple LLVM 9.0.0 (clang-900.0.39.2)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> import logging >>> logging.info("Hi Kellen") >>> >>>

Ahh, sorry. Default is warning.

marcoabreu · 2018-08-03T11:30:10Z

Jenkinsfile

-                del /S /Q ${env.WORKSPACE}\\pkg_vc14_cpu\\python\\*.pyc
-                C:\\mxnet\\test_cpu.bat"""
+              unstash 'windows_package_cpu'
+              powershell 'ci/windows/test_py2_cpu.ps1'


Would it be possible to bring it into the same format as our runtime functions on unix where we have all runtime functions (builds and tests) defined in that one file and it then goes into multiple branches? This allows us to be consistent in the methodology and you can see everything by looking at one file.

Do you know how to do this in powershell? it would take some time for marginal benefits right now. I'm open to do so in the future, but not inclined to do it further in this PR.

marcoabreu · 2018-08-03T11:31:49Z

ci/build_windows.py

+    WIN_GPU = 'WIN_GPU'
+    WIN_GPU_MKLDNN = 'WIN_GPU_MKLDNN'
+
+CMAKE_FLAGS = {


That looks nice!

marcoabreu · 2018-08-03T11:33:25Z

ci/build_windows.py

+    with remember_cwd():
+        os.chdir(path)
+        logging.info("Generating project with CMake")
+        check_call("cmake -G \"Visual Studio 14 2015 Win64\" {} {}".format(CMAKE_FLAGS[args.flavour], mxnet_root), shell=True)


Should we maybe take "Visual Studio 14 2015 Win64" and put it into a constant at the top? This will allow us to see which compilers we're aiming for.

good point, I would like to address this in the future, also when we update to vs2017. I tested locally and worked fine.

marcoabreu · 2018-08-03T11:34:54Z

ci/build_windows.py

+        copy_tree(j('3rdparty','mshadow', 'mshadow'), j(pkgdir, 'include', 'mshadow'))
+        copy_tree(j('3rdparty','tvm','nnvm', 'include'), j(pkgdir,'include', 'nnvm', 'include'))
+        logging.info("Compressing package: %s", pkgfile)
+        check_call(['7z', 'a', pkgfile, pkgdir])


Would it be possible to use a built-in python mechanism to compress the files opposed to requiring 7zip as a dependency?

possibly. We were using 7z before, seems it's more widespread in windows than targz. Didn't want to change this functionality ATM.

marcoabreu · 2018-08-03T11:35:40Z

ci/build_windows.py

+        check_call(['7z', 'a', pkgfile, pkgdir])
+
+
+def nix_build(args):


Could you add some documentation here? I don't understand what nix_build means here - my first thought would be "unix based build", but that wouldn't make sense in this context

this code would be extended in the future, is supposed to be used only on windows for now and from CI, once it's ready for users we will add it to the documentation.

marcoabreu · 2018-08-03T11:37:04Z

ci/build_windows.py

+    if system == 'Windows':
+        logging.info("Detected Windows platform")
+        if 'OpenBLAS_HOME' not in os.environ:
+            os.environ["OpenBLAS_HOME"] = "C:\\mxnet\\openblas"


Shall we maybe print a message when we auto-assume a path? Otherwise the error might say that C:\mxnet\openblas is missing although it actually means that people didn't set OpenBLAS_HOME properly. It might confuse users when they think it has to be at a specific path.

This will be adressed in a future PR when we handle dependencies, right now is too much for a single PR.

marcoabreu · 2018-08-03T11:37:53Z

ci/build_windows.py

+            os.environ["CUDA_PATH"] = "C:\\CUDA\\v8.0"
+        windows_build(args)
+
+    elif system == 'Linux' or system == 'Darwin':


Ah, now I get it. But why are we offering unix compilation in a file which is only for windows compilation? Seems like we're mixing concerns here a bit.

this is not exposed to users right now.

marcoabreu · 2018-08-03T11:39:33Z

ci/windows/test_py2_cpu.ps1

+# specific language governing permissions and limitations
+# under the License.
+
+7z x -y windows_package.7z


What happens if a user runs this script two times in a row? Would it override during unpacking, skip silently or throw an error? It seems like there is a hard requirement for the 7z file here, although some people might want to compile themselves and then just run the tests

works fine.

marcoabreu · 2018-08-03T11:41:12Z

ci/windows/test_py2_cpu.ps1

+7z x -y windows_package.7z
+$env:PYTHONPATH=join-path $pwd.Path windows_package\python
+$env:MXNET_STORAGE_FALLBACK_LOG_VERBOSE=0
+c:\Anaconda3\envs\py2\Scripts\pip install -r tests\requirements.txt


Do you think it makes sense to auto-detect the path (maybe from PATH) instead of hardcoding it?

I have the feeling is not easy to do so reliably. This makes sure you are in that virtualenv. Also difficult to set virtualenv from powershell. We can refine further, but I don't want to play with this any further ATM.

marcoabreu · 2018-08-03T11:42:27Z

tests/python/gpu/test_forward.py


 def _get_model():
    if not os.path.exists('model/Inception-7-symbol.json'):
-        download('http://data.mxnet.io/models/imagenet/inception-v3.tar.gz', dirname='model')


Are you changing the behaviour here? It seems like we are not writing to model/ but into the root.

it works as expected, tar is not available on windows. The test would fail otherwise.

I mean that we are now extracting to a different path (the root of the mxnet source) opposed to the subdirectory "models".

marcoabreu · 2018-08-03T12:05:55Z

Comments will be addressed in a future PR

* Windows scripted build Adjust Jenkins builds to use ci/build_windows.py Issues: apache#8714 apache#11100 apache#10166 apache#10049 * Fix bug * Fix non-portable ut * add xunit

[MXNET-750] fix nested call on CachedOp. (apache#11951) * fix nested call on cachedop. * fix. extend reshape op to allow reverse shape inference (apache#11956) Improve sparse embedding index out of bound error message; (apache#11940) [MXNET-770] Remove fixed seed in flaky test (apache#11958) * Remove fixed seed in flaky test * Remove fixed seed in flaky test Update ONNX docs with the latest supported ONNX version (apache#11936) Reduced test to 3 epochs and made gpu only (apache#11863) * Reduced test to 3 epochs and made GPU only * Moved logger variable so that it's accessible Fix flaky tests for test_laop_4 (apache#11972) Updating R client docs (apache#11954) * Updating R client docs * Forcing build Fix install instructions for MXNET-R (apache#11976) * fix install instructions for MXNET-R * fix install instructions for MXNET-R * fix default cuda version for MXNet-R [MXNET-751] fix ce_loss flaky (apache#11971) * add xavier initializer * remove comment line [MXNET-769] set MXNET_HOME as base for downloaded models through base.data_dir() (apache#11636) * set MXNET_DATA_DIR as base for downloaded models through base.data_dir() push joblib to save containers so is not required when running * MXNET_DATA_DIR -> MXNET_HOME [MXNET-748] linker fixed on Scala issues (apache#11989) * put force load back as a temporary solution * use project.basedir as relative path for OSX linker [MXNET-772] Re-enable test_module.py:test_module_set_params (apache#11979) [MXNET-771] Fix Flaky Test test_executor.py:test_dot (apache#11978) * use assert_almost_equal, increase rtol, reduce matrix size * remove seed in test_bind * add seed 0 to test_bind, it is still flaky * add comments for tracking remove mod from arity 2 version of load-checkpoint in clojure-package (apache#11808) * remove mod from arity 2 version of load-checkpoint * load-checkpoint arity 2 test Add unit test stage for mxnet cpu in debug mode (apache#11974) Website broken link fixes (apache#12014) * fix broken link * fix broken link * switch to .md links * fix broken link removed seed from flaky test (apache#11975) Disable ccache log print due to threadunsafety (apache#11997) Added default tolerance levels for regression checks for MBCC (apache#12006) * Added tolerance level for assert_almost_equal for MBCC * Nudge to CI Disable flaky mkldnn test_requantize_int32_to_int8 (apache#11748) [MXNET-769] Usability improvements to windows builds (apache#11947) * Windows scripted build Adjust Jenkins builds to use ci/build_windows.py Issues: apache#8714 apache#11100 apache#10166 apache#10049 * Fix bug * Fix non-portable ut * add xunit Fix import statement (apache#12005) array and multiply are undefined. Importing them from ndarray Disable flaky test test_random.test_gamma_generator (apache#12022) [MXNET-770] Fix flaky test: test_factorization_machine_module (apache#12023) * Remove fixed seed in flaky test * Remove fixed seed in flaky test * Update random seed to reproduce the issue * Fix Flaky unit test and add a training test * Remove fixed seed in flaky test * Update random seed to reproduce the issue * Fix Flaky unit test and add a training test * Increase accuracy check disable opencv threading for forked process (apache#12025) Bug fixes in control flow operators (apache#11942) Fix data narrowing warning on graph_executor.cc (apache#11969) Fix flaky tests for test_squared_hinge_loss (apache#12017) Fix flaky tests for test_hinge_loss (apache#12020) remove fixed seed for test_sparse_ndarray/test_operator_gpu.test_sparse_nd_pickle (apache#12012) Removed fixed seed from , test_loss:test_ctc_loss_train (apache#11985) Removed fixed seed from , test_loss:test_sample_weight_loss (apache#11986) Fix reduce_kernel_M1 (apache#12026) * Fix reduce_kernel_M1 * Improve test_norm Update test_loss.py to remove fixed seed (apache#11995) [MXNET-23] Adding support to profile kvstore server during distributed training (apache#11215) * server profiling merge with master cleanup old code added a check and better info message add functions for C compatibility fix doc lint fixes fix compile issues lint fix build error update function signatures to preserve compatibility fix comments lint * add part1 of test * add integration test Re-enabling test_ndarray/test_cached (apache#11950) Test passes on CPU and GPU (10000 runs) make gluon rnn layers hybrid blocks (apache#11482) * make Gluon RNN layer hybrid block * separate gluon gpu tests * remove excess assert_raises_cudnn_disabled usage * add comments and refactor * add bidirectional test * temporarily remove hybridize in test_gluon_rnn.test_layer_fill_shape [MXNET-751] fix bce_loss flaky (apache#11955) * add fix to bce_loss * add comments * remove unecessary comments Doc fix for a few optimizers (apache#12034) * Update optimizer.py * Update optimizer.py

* Windows scripted build Adjust Jenkins builds to use ci/build_windows.py Issues: apache#8714 apache#11100 apache#10166 apache#10049 * Fix bug * Fix non-portable ut * add xunit

larroy requested a review from marcoabreu as a code owner July 31, 2018 15:53

larroy force-pushed the windows branch 2 times, most recently from f76fe6a to 5e66751 Compare July 31, 2018 15:57

larroy force-pushed the windows branch 9 times, most recently from 795fc84 to 6c8c3a1 Compare August 1, 2018 10:21

larroy requested a review from szha as a code owner August 1, 2018 10:21

larroy force-pushed the windows branch 7 times, most recently from 5d2026b to 0e69073 Compare August 1, 2018 14:20

larroy changed the title ~~[WIP] Usability improvements to windows builds~~ [WIP][MXNET-769] Usability improvements to windows builds Aug 1, 2018

larroy force-pushed the windows branch from 0e69073 to 14f9499 Compare August 1, 2018 14:47

larroy force-pushed the windows branch from 312c5c1 to 067b5ed Compare August 1, 2018 18:45

larroy added 2 commits August 2, 2018 11:00

Windows scripted build

b94262f

Adjust Jenkins builds to use ci/build_windows.py Issues: apache#8714 apache#11100 apache#10166 apache#10049

Fix bug

782fc0e

larroy force-pushed the windows branch from 086c472 to 94a77a5 Compare August 2, 2018 09:04

Fix non-portable ut

a6d4c62

larroy force-pushed the windows branch from 94a77a5 to a6d4c62 Compare August 2, 2018 11:36

add xunit

873ceeb

larroy force-pushed the windows branch from 20a9940 to 873ceeb Compare August 2, 2018 15:50

larroy changed the title ~~[WIP][MXNET-769] Usability improvements to windows builds~~ [MXNET-769] Usability improvements to windows builds Aug 2, 2018

KellenSunderland reviewed Aug 2, 2018

View reviewed changes

KellenSunderland approved these changes Aug 2, 2018

View reviewed changes

marcoabreu suggested changes Aug 3, 2018

View reviewed changes

marcoabreu merged commit 1fa04f2 into apache:master Aug 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MXNET-769] Usability improvements to windows builds #11947

[MXNET-769] Usability improvements to windows builds #11947

larroy commented Jul 31, 2018 •

edited

Loading

marcoabreu commented Jul 31, 2018

larroy commented Aug 1, 2018

larroy commented Aug 1, 2018

KellenSunderland left a comment

KellenSunderland Aug 2, 2018

larroy Aug 2, 2018 •

edited

Loading

KellenSunderland Aug 2, 2018

KellenSunderland Aug 2, 2018

larroy Aug 2, 2018 •

edited

Loading

KellenSunderland Aug 2, 2018

marcoabreu Aug 3, 2018

larroy Aug 3, 2018

marcoabreu Aug 3, 2018

marcoabreu Aug 3, 2018

larroy Aug 3, 2018 •

edited

Loading

marcoabreu Aug 3, 2018

larroy Aug 3, 2018 •

edited

Loading

marcoabreu Aug 3, 2018

larroy Aug 3, 2018

marcoabreu Aug 3, 2018

larroy Aug 3, 2018

marcoabreu Aug 3, 2018

larroy Aug 3, 2018

marcoabreu Aug 3, 2018

larroy Aug 3, 2018

marcoabreu Aug 3, 2018

larroy Aug 3, 2018

marcoabreu Aug 3, 2018

larroy Aug 3, 2018

marcoabreu Aug 3, 2018

marcoabreu commented Aug 3, 2018

		def buildir() -> str:
		return os.path.join(get_mxnet_root(), "build")

		check_call(['7z', 'a', pkgfile, pkgdir])


		def nix_build(args):

[MXNET-769] Usability improvements to windows builds #11947

[MXNET-769] Usability improvements to windows builds #11947

Conversation

larroy commented Jul 31, 2018 • edited Loading

Description

Checklist

Essentials

marcoabreu commented Jul 31, 2018

larroy commented Aug 1, 2018

larroy commented Aug 1, 2018

KellenSunderland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larroy Aug 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larroy Aug 2, 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

larroy Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larroy Aug 3, 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

Choose a reason for hiding this comment

marcoabreu commented Aug 3, 2018

larroy commented Jul 31, 2018 •

edited

Loading

larroy Aug 2, 2018 •

edited

Loading

larroy Aug 2, 2018 •

edited

Loading

larroy Aug 3, 2018 •

edited

Loading

larroy Aug 3, 2018 •

edited

Loading