[MXNET-769] set MXNET_HOME as base for downloaded models through base.data_dir() #11636

larroy · 2018-07-10T21:48:55Z

Description

This allows setting the data dir for models as an environment variable and with better per platform defaults.

Would also solve the following issue:
#11616

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

szha · 2018-07-10T22:26:05Z

docs/faq/env_var.md

@@ -126,6 +126,11 @@ When USE_PROFILER is enabled in Makefile or CMake, the following environments ca
  - Values: String ```(default='https://apache-mxnet.s3-accelerate.dualstack.amazonaws.com/'```
  - The repository url to be used for Gluon datasets and pre-trained models.

+* MXNET_DATA_DIR


There are models with such default path too. Rename to MXNET_HOME?

can you elaborate? I is this path hardcoded somewhere else?

Apologies for the confusion. What I meant is we can use a variable name such as MXNET_HOME so that other assets such as models and datasets can all be saved there.

okay, I don't see what is the different semantic with dada dir and home, might be my poor english skills, so fine to take the suggestion.

szha · 2018-07-10T22:27:30Z

python/mxnet/base.py

+        if 'XDG_DATA_HOME'in os.environ:
+            return os.path.join(os.environ['XDG_DATA_HOME'], 'mxnet')
+        else:
+            return os.path.join(os.path.expanduser("~"), '.local', 'share', 'mxnet')


Please don't change the default value from $HOME/.mxnet/ or otherwise we'd be forcing everyone to re-download.

is it a big deal? we can also migrate the directory...

Yes, it is a big deal.

szha · 2018-07-10T22:28:57Z

python/mxnet/base.py

+    if system == 'Windows':
+        return os.environ.get('APPDATA')
+    elif system == 'Linux' or system == 'Darwin':
+        if 'XDG_DATA_HOME'in os.environ:


what's XDG? why?

freedesktop convention https://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html

Have we ever followed this spec anywhere? It doesn't seem necessary.

marcoabreu

Could you please elaborate how this solves the issue? The problem is that the access to the same directory and file is not synchronized. Since we want people to use MXNet in multi tenancy mode (without having to redownload the models for every single instance), we should add synchronisation.
This solution would only fix the problem if people set a unique path for every process.

larroy · 2018-07-11T09:11:08Z

@marcoabreu this is the first step in the right direction. It solves the problem if you have it by specifying different directories. Also allow to run unit tests in parallel in ci. This should be done before any other complexities such as portable cross system wide locks which are not trivial. Windows is not fully POSIX

larroy · 2018-07-11T12:53:18Z

docs/faq/env_var.md

+* MXNET_DATA_DIR
+  - Data directory in the filesystem for storage, for example when downloading gluon models.
+  - Default in *nix is .local/share/mxnet APPDATA/mxnet in windows, XDG_DATA_HOME/mxnet if
+    XDG_DATA_HOME is set.


I will remove XDG_DATA_HOME

Can you remove it?

larroy · 2018-08-01T14:18:17Z

ready to merge

marcoabreu · 2018-08-01T14:16:51Z

docs/faq/env_var.md

+* MXNET_DATA_DIR
+  - Data directory in the filesystem for storage, for example when downloading gluon models.
+  - Default in *nix is .local/share/mxnet APPDATA/mxnet in windows, XDG_DATA_HOME/mxnet if
+    XDG_DATA_HOME is set.


Can you remove it?

marcoabreu · 2018-08-01T14:17:43Z

python/mxnet/base.py

+
+    :return: data directory in the filesystem for storage, for example when downloading models
+    """
+    return os.getenv('MXNET_DATA_DIR', data_dir_default())


Could you please rename as requested by Sheng?

No, scala package already using MXNET_DATA_DIR, so seems it's the convention.

@szha @nswamy could you help us out here?

What help do we need?

MXNET_DATA_DIR is the place where we download models/training/test/other data.
MXNET_HOME sounds like a place where you have library, for example if you want set it to PYTHONPATH

Scala should follow the previous convention instead of making up new ones.

@yzhliu @lanking520

I am not sure if MXNET_DATA_DIR being used on our side, but we do have this:
link

This variable already existed in some examples when I started working
https://github.com/apache/incubator-mxnet/blob/5627bc3ab71449532aa42297827993353d9ea580/scala-package/examples/src/main/scala/org/apache/mxnetexamples/imclassification/TrainMnist.scala#L115

I have no preference one way or another as long as we consistently follow the same.

For the library we have MXNET_LIBRARY_PATH as can be seen in libinfo.py
@szha where is the MXNET_HOME convention? can you point out files?

marcoabreu · 2018-08-01T14:20:08Z

python/mxnet/test_utils.py

@@ -484,7 +484,7 @@ def assert_almost_equal(a, b, rtol=None, atol=None, names=('a', 'b'), equal_nan=
        return

    index, rel = find_max_violation(a, b, rtol, atol)
-    np.set_printoptions(threshold=4, suppress=True)
+    np.set_printoptions(threshold=np.nan, suppress=True)


Could you revert this please as it is outside the scope of this PR?

marcoabreu · 2018-08-01T14:21:06Z

python/mxnet/util.py

@@ -0,0 +1,30 @@
+# Licensed to the Apache Software Foundation (ASF) under one


I didn't check, but don't we already have a place for utility functions?

I looked for it, but I think we don't.

marcoabreu · 2018-08-01T14:21:18Z

python/mxnet/util.py

+    available in Python2"""
+    if sys.version_info[0] < 3:
+        from distutils.dir_util import mkpath
+        mkpath(d)


Am I assuming right that this function is a NO-OP if the dir exists?

marcoabreu · 2018-08-01T14:21:34Z

src/executor/graph_executor.cc

@@ -1282,7 +1282,7 @@ void GraphExecutor::InitDataEntryMemory(std::vector<NDArray>* shared_pool) {
  for (size_t i = 0; i < pool_info.size(); i++) {
    sorted_pool_index.push_back(i);
  }
-  auto pool_comparator = [&pool_info](int lhs, int rhs){
+  auto pool_comparator = [&pool_info](size_t lhs, size_t rhs){


Could you revert this please as it is outside the scope of this PR?

fixes a warning, minor change, why revert?

To keep concerns separate

marcoabreu · 2018-08-01T14:21:47Z

tests/python/unittest/test_base.py

@@ -0,0 +1,50 @@
+# Licensed to the Apache Software Foundation (ASF) under one


larroy · 2018-08-01T14:30:01Z

Addressed comments.

szha

we can't use MXNET_DATA_DIR

larroy · 2018-08-01T17:58:21Z

@szha grep for MXNET_HOME, is used for where the root folder of mxnet source is located. This seems ambiguous and confusing for users. Can you elaborate why MXNET_HOME is better than MXNET_DATA_DIR?

push joblib to save containers so is not required when running

larroy · 2018-08-01T21:08:56Z

@szha merge?

marcoabreu · 2018-08-01T21:42:24Z

If you think mxnet home is already occupied for a different purpose, feel free to propose an alternative.

larroy · 2018-08-01T22:13:24Z

@marcoabreu did you see the changes?

marcoabreu · 2018-08-01T22:23:37Z

Yes. I am concerned that we are breaking backwards compatibility if we merge it as-is.

I personally don't think that mxnet home is a good variable if our convention is that it points to the source (I didn't check, just judging from the conversation here). We dont want the models to be stored in the git managed repository or pollute the workspace.

szha · 2018-08-02T03:26:36Z

Other usages of MXNET_HOME are not user facing as they are only used internally in scripts such as installation.

….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-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

….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

larroy requested a review from szha as a code owner July 10, 2018 21:48

larroy force-pushed the data_dir branch from addd518 to 1b6779b Compare July 10, 2018 22:14

szha reviewed Jul 10, 2018

View reviewed changes

larroy force-pushed the data_dir branch 2 times, most recently from a4273ce to aff84a1 Compare July 10, 2018 23:10

marcoabreu reviewed Jul 11, 2018

View reviewed changes

larroy commented Jul 11, 2018

View reviewed changes

larroy force-pushed the data_dir branch 3 times, most recently from 0d8e575 to 0077502 Compare July 18, 2018 19:11

larroy force-pushed the data_dir branch 7 times, most recently from 4cebf87 to eb718d5 Compare August 1, 2018 12:11

larroy requested a review from anirudh2290 as a code owner August 1, 2018 12:11

larroy changed the title ~~[WIP] set MXNET_DATA_DIR as base for downloaded models through base.data_dir()~~ set MXNET_DATA_DIR as base for downloaded models through base.data_dir() Aug 1, 2018

marcoabreu suggested changes Aug 1, 2018

View reviewed changes

larroy force-pushed the data_dir branch from eb718d5 to e9a3d95 Compare August 1, 2018 14:29

larroy changed the title ~~set MXNET_DATA_DIR as base for downloaded models through base.data_dir()~~ [MXNET-769] set MXNET_DATA_DIR as base for downloaded models through base.data_dir() Aug 1, 2018

larroy force-pushed the data_dir branch from e9a3d95 to 2db105c Compare August 1, 2018 16:49

szha suggested changes Aug 1, 2018

View reviewed changes

larroy added 2 commits August 1, 2018 21:40

set MXNET_DATA_DIR as base for downloaded models through base.data_dir()

8e12d73

push joblib to save containers so is not required when running

MXNET_DATA_DIR -> MXNET_HOME

c86c09c

larroy force-pushed the data_dir branch from 2db105c to c86c09c Compare August 1, 2018 19:42

larroy requested a review from yzhliu as a code owner August 1, 2018 19:42

larroy changed the title ~~[MXNET-769] set MXNET_DATA_DIR as base for downloaded models through base.data_dir()~~ [MXNET-769] set MXNET_HOME as base for downloaded models through base.data_dir() Aug 1, 2018

szha approved these changes Aug 2, 2018

View reviewed changes

marcoabreu approved these changes Aug 2, 2018

View reviewed changes

marcoabreu merged commit 564e01a into apache:master Aug 2, 2018

larroy deleted the data_dir branch November 15, 2018 18:44

		@@ -0,0 +1,30 @@
		# Licensed to the Apache Software Foundation (ASF) under one

		@@ -0,0 +1,50 @@
		# Licensed to the Apache Software Foundation (ASF) under one

[MXNET-769] set MXNET_HOME as base for downloaded models through base.data_dir() #11636

[MXNET-769] set MXNET_HOME as base for downloaded models through base.data_dir() #11636

Conversation

larroy commented Jul 10, 2018 • edited Loading

Description

Checklist

Essentials

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 left a comment • edited Loading

Choose a reason for hiding this comment

larroy commented Jul 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larroy commented Aug 1, 2018

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 1, 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

larroy commented Aug 1, 2018

szha left a comment

Choose a reason for hiding this comment

larroy commented Aug 1, 2018

larroy commented Aug 1, 2018

marcoabreu commented Aug 1, 2018

larroy commented Aug 1, 2018

marcoabreu commented Aug 1, 2018 • edited Loading

szha commented Aug 2, 2018

larroy commented Jul 10, 2018 •

edited

Loading

marcoabreu left a comment •

edited

Loading

larroy Aug 1, 2018 •

edited

Loading

marcoabreu commented Aug 1, 2018 •

edited

Loading