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

refactor kvstore on model.py #263

Merged
merged 19 commits into from
Oct 11, 2015
Merged

refactor kvstore on model.py #263

merged 19 commits into from
Oct 11, 2015

Conversation

mli
Copy link
Contributor

@mli mli commented Oct 11, 2015

replaced kvstore_type and udpate_on_server by kvstore (either a type or a KVStore) on model.py, more doc

https://github.com/mli/mxnet/blob/master/doc/developer-guide/multi_node.md

passed tests on mnist and cifar with multi-gpus

mli
mli reviewed Oct 11, 2015
View reviewed changes
python/mxnet/context.py
devtype2str = {1: 'cpu', 2: 'gpu'}
devstr2type = {'cpu': 1, 'gpu': 2}
devtype2str = {1: 'cpu', 2: 'gpu', 3: 'cpu_pinned'}
devstr2type = {'cpu': 1, 'gpu': 2, 'cpu_pinned': 3}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tqchen please double check if it is ok to have cpu_pinned

Copy link
Member

Choose a reason for hiding this comment

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

Let us not expose this for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

must be here, create state may create pinned mem array. That bug costs me a
hour to find

On Sun, Oct 11, 2015 at 1:45 AM Tianqi Chen [email protected]
wrote:

In python/mxnet/context.py
#263 (comment):

@@ -29,8 +29,8 @@ class Context(object):
"""
# static class variable
default_ctx = None

  • devtype2str = {1: 'cpu', 2: 'gpu'}
  • devstr2type = {'cpu': 1, 'gpu': 2}
  • devtype2str = {1: 'cpu', 2: 'gpu', 3: 'cpu_pinned'}
  • devstr2type = {'cpu': 1, 'gpu': 2, 'cpu_pinned': 3}

Let us not expose this for now

—
Reply to this email directly or view it on GitHub
https://github.com/dmlc/mxnet/pull/263/files#r41705789.

Copy link
Member

Choose a reason for hiding this comment

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

I see what you mean. Keep it here then

@tqchen
Copy link
Member

tqchen commented Oct 11, 2015

@mli Please test all the modes in kvstore

tqchen
tqchen reviewed Oct 11, 2015
View reviewed changes
include/mxnet/kvstore.h
/**
* \brief return the type
*/
inline std::string type() { return type_; }
Copy link
Member

Choose a reason for hiding this comment

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

This is very bad, as you return the space get destructed, and python side get a invalid space pointer. Return const std::string & instead

@tqchen
Copy link
Member

tqchen commented Oct 11, 2015

Three comments in general

  • Please fix the BUG on kvstore type return, and python side string wrapping.
  • Remove commented lines
  • Use python @property decorator to make the getters shorter

@tqchen
Copy link
Member

tqchen commented Oct 11, 2015

On updater index hack for update_on_kvstore=False. It would be nice if we can use the old way.

So in future we can use index to tell which weight is which(one reason we created it in this way)

tqchen
tqchen reviewed Oct 11, 2015
View reviewed changes
python/mxnet/optimizer.py
@@ -148,8 +148,7 @@ def create(name, rescale_grad=1, **kwargs):
else:
raise ValueError('Cannot find optimizer %s' % name)


def optimizer_clossure(optimizer):
def get_updater(optimizer):
Copy link
Member

Choose a reason for hiding this comment

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

create is a better name, as it creates a updater closure each time

mli added a commit that referenced this pull request Oct 11, 2015
@mli
Merge pull request #263 from mli/master
49a0ff6 
refactor kvstore on model.py
@mli mli merged commit 49a0ff6 into apache:master Oct 11, 2015
stefanhenneking pushed a commit to stefanhenneking/mxnet that referenced this pull request Jun 30, 2017
@ptrendx @piiswrong
Adding __syncwarp() from CUDA 9 to unsafe warp-synchronous code (apac…
83a3b45 
…he#263)
eric-haibin-lin pushed a commit to eric-haibin-lin/mxnet that referenced this pull request Dec 2, 2017
@superbobry @tqchen
Added missing include to hdfs_filesys.cc (apache#263)
301c9e5 
std::min requires algorithm header as per the standard. It seems to
work without it on Linux/OS X but breaks on Windows.
joseph-wakeling-sociomantic added a commit to joseph-wakeling-sociomantic/mxnet that referenced this pull request Jan 15, 2018
@joseph-wakeling-sociomantic
Merge stable 1.0.0 release of MXNet
e5e1064 
* cub 89de7ab2(89de7ab2)...05eb57fa(05eb57fa) (798 commits)
  > Merge pull request sociomantic-tsunami#1 from ptrendx/update
  > Create README.md
  > update readme.md
  > 1.7.0
  < 1.6.4 doc update (part 2)
  (...)

* dlpack ()...a6e09b5(a6e09b5) (1 commits)
  > Change order of device_type/id in Context (sociomantic-tsunami#11)

* dmlc-core a6c5701(a6c5701)...87b7ffa(87b7ffa) (54 commits)
  > add SetEnv (apache#322)
  > Fix a bug in seek/tell on Windows (apache#318)
  > Fixes apache#303: added recurse_directories to InputSplit::Create (apache#310)
  > Type name error (apache#316)
  > Small param bug (apache#315)
  (...)

* mshadow c037b06(c037b06)...2d7780c(2d7780c) (42 commits)
  > [CMAKE][ARM] Change USE_SSE to SUPPORT_MSSE2 to it uses the autodetected presence of sse compiler flag from the parent project (see PR apache#8395) (apache#303)
  > Makes repeated setting of gpu rng seed produce repeatable sequences. (apache#304)
  > Add USE_SSE which propagates into MSHADOW_USE_SSE in cmake (apache#302)
  > fix range (apache#301)
  > fix for random seed generation (apache#300)
  (...)

* nnvm b279286(b279286)...e4a138a(e4a138a) (139 commits)
  > [TVM] upgrade to latest version (apache#263)
  > Added support for CoreML Permute layers (apache#262)
  > [CMPL] Add Support for Other Data Types (apache#252)
  > fix onnx conv2d_transpose loading (apache#245)
  > [FIX] Fix from_mxnet for multiple outputs symbol (apache#247)
  (...)

* ps-lite v1+118(acdb698)...v1+123(2ce8b9a) (2 commits)
  > Merge pull request apache#117 from madjam/listen-interface
  > Merge pull request apache#109 from b0noI/master
joseph-wakeling-sociomantic added a commit to joseph-wakeling-sociomantic/mxnet that referenced this pull request Jan 15, 2018
@joseph-wakeling-sociomantic
Merge stable 1.0.0 release of MXNet
2f3fc17 
Fixes sociomantic-tsunami#11

* cub 89de7ab2(89de7ab2)...05eb57fa(05eb57fa) (798 commits)
  > Merge pull request sociomantic-tsunami#1 from ptrendx/update
  > Create README.md
  > update readme.md
  > 1.7.0
  < 1.6.4 doc update (part 2)
  (...)

* dlpack ()...a6e09b5(a6e09b5) (1 commits)
  > Change order of device_type/id in Context (sociomantic-tsunami#11)

* cub 89de7ab2(89de7ab2)...05eb57fa(05eb57fa) (798 commits)
  > Merge pull request sociomantic-tsunami#1 from ptrendx/update
  > Create README.md
  > update readme.md
  > 1.7.0
  < 1.6.4 doc update (part 2)
  (...)

* dlpack ()...a6e09b5(a6e09b5) (1 commits)
  > Change order of device_type/id in Context (sociomantic-tsunami#11)

* dmlc-core a6c5701(a6c5701)...87b7ffa(87b7ffa) (54 commits)
  > add SetEnv (apache#322)
  > Fix a bug in seek/tell on Windows (apache#318)
  > Fixes apache#303: added recurse_directories to InputSplit::Create (apache#310)
  > Type name error (apache#316)
  > Small param bug (apache#315)
  (...)

* mshadow c037b06(c037b06)...2d7780c(2d7780c) (42 commits)
  > [CMAKE][ARM] Change USE_SSE to SUPPORT_MSSE2 to it uses the autodetected presence of sse compiler flag from the parent project (see PR apache#8395) (apache#303)
  > Makes repeated setting of gpu rng seed produce repeatable sequences. (apache#304)
  > Add USE_SSE which propagates into MSHADOW_USE_SSE in cmake (apache#302)
  > fix range (apache#301)
  > fix for random seed generation (apache#300)
  (...)

* nnvm b279286(b279286)...e4a138a(e4a138a) (139 commits)
  > [TVM] upgrade to latest version (apache#263)
  > Added support for CoreML Permute layers (apache#262)
  > [CMPL] Add Support for Other Data Types (apache#252)
  > fix onnx conv2d_transpose loading (apache#245)
  > [FIX] Fix from_mxnet for multiple outputs symbol (apache#247)
  (...)

* ps-lite v1+118(acdb698)...v1+123(2ce8b9a) (2 commits)
  > Merge pull request apache#117 from madjam/listen-interface
  > Merge pull request apache#109 from b0noI/master
@joseph-wakeling-sociomantic joseph-wakeling-sociomantic mentioned this pull request Jan 15, 2018
Merged
eric-haibin-lin pushed a commit to eric-haibin-lin/mxnet that referenced this pull request Apr 4, 2018
@tqchen
[TVM] upgrade to latest version (apache#263)
e4a138a
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
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
@mli @tqchen @niuchenglei