Change class variables to thread local variables by anirudh2290 · Pull Request #10833 · apache/mxnet

anirudh2290 · 2018-05-07T02:22:42Z

Description

There are many places where class variables which end up modifying the state from multiple threads and causing issues like the one here: #9920 .
This PR is currently WIP because more investigation is required on test_multi_worker_forked_data_loader and test_laop_3(able to reproduce this test failure without my changes also). Also, more tests need to be added.

@piiswrong

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

Changes

Feature1, tests, (and when applicable, API doc)
Feature2, tests, (and when applicable, API doc)

Comments

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

szha · 2018-05-07T17:31:38Z

    gpu(1)
    """
-    # static class variable
-    default_ctx = None


This will break a lot of existing code. Is this change intended for 2.0?

I don't think this was meant to be public API. current_context() is used for accessing this.

@piiswrong told me that default_ctx, devtype2str and devstr2type were not meant to be public API. This will also be renamed to _default_ctx. Users are expected to use current_context or default_context in test_utils. I agree though that this will break some existing code. I was of the opinion that the release after 1.2 is 2.0 but I am not sure.

If there are users who depend on this, we can make a backward compatible wrapper with classmethod and property:https://stackoverflow.com/questions/1697501/python-staticmethod-with-property

_default_ctx = threading.local() @property @classmethod def default_ctx(): return _default_ctx.value

piiswrong · 2018-05-07T17:54:52Z

+        def __exit__(self, ptype, value, trace):
+            self.load_local_var()
+else:
+    class ThreadLocalRegistry(object, metaclass=ThreadLocalRegistryMeta):


This looks really complicated. why do you have to use inheritance? Isn't composition better?

Inheritance was used since the thread local variables need to still be bound to the class rather than the object.

piiswrong · 2018-05-07T17:56:21Z

+    class ThreadLocalRegistry(object, metaclass=ThreadLocalRegistryMeta):
+        """Structure that holds threadlocal variables and sets context manager"""
+
+        def __init__(self, name):


why do you need a name?

The name here is the name of the threadlocal variable for example default_ctx.
I will add an empty or None check here, to prevent from being misused.

piiswrong · 2018-05-07T18:32:20Z

    kwargs
        The attributes to set for all symbol creations in the scope.
    """
-    current = None


_current = threading.local()

piiswrong · 2018-05-07T18:33:02Z


    def __enter__(self):
        # pylint: disable=protected-access
-        self._old_scope = AttrScope.current


self._old_scope = AttrScope.current.value

piiswrong · 2018-05-07T18:33:23Z


    def __exit__(self, ptype, value, trace):
-        assert self._old_scope
-        AttrScope.current = self._old_scope


AttrScope.current.value = self._old_scope

piiswrong · 2018-05-07T18:37:21Z

    gpu(1)
    """
-    # static class variable
-    default_ctx = None


_default_ctx = threading.local() @property @classmethod def default_ctx(): return _default_ctx.value

…l_ctx

piiswrong · 2018-05-10T04:27:11Z

    def __enter__(self):
-        self._old_ctx = Context.default_ctx
-        Context.default_ctx = self
+        if not hasattr(Context._default_ctx, "value"):


why do you need this? Isn't it set with https://github.com/apache/incubator-mxnet/pull/10833/files#diff-1a5e06031378f44204fd1da1fffc0b07R145

what about a situation like below:

import threading import mxnet as mx def test_context(): ctx_list = [] def f(): mx.context.Context.default_ctx ctx_list.append(Context.default_ctx) thread = threading.Thread(target=f) thread.start() thread.join() if __name__ == '__main__': test_context()

Without hasattr this doesn't work.

piiswrong · 2018-05-10T04:28:07Z

    def create(prefix, params, hint):
        """Creates prefix and params for new `Block`."""
-        current = _BlockScope._current
+        current = getattr(_BlockScope._current, "value", None)


initialize _BlockScope._current.value to None globally and use _BlockScope._current.value directly here?

piiswrong · 2018-05-10T04:28:16Z

    def __enter__(self):
-        self._old_manager = NameManager.current
-        NameManager.current = self
+        if not hasattr(NameManager._current, "value"):


piiswrong · 2018-05-10T04:28:27Z

+        warnings.warn("NameManager.current has been deprecated. "
+                      "It is advised to use the `with` statement with NameManager.",
+                      DeprecationWarning)
+        if not hasattr(NameManager._current, "value"):


piiswrong · 2018-05-10T04:29:23Z

    ctx : Context, optional
        An optional device context.
-        Defaults to the current default context (``mxnet.Context.default_ctx``).
+        Defaults to the current default context (``mxnet.Context._default_ctx.value``).


This is public API. change to mxnet.current_context

piiswrong · 2018-05-10T04:30:27Z

        # pylint: disable=protected-access
-        self._old_scope = AttrScope.current
-        attr = AttrScope.current._attr.copy()
+        if not hasattr(AttrScope._current, "value"):


piiswrong · 2018-05-10T04:31:05Z

Please remove the hasatter/getattr statements. Otherwise LGTM

anirudh2290 · 2018-05-11T07:19:29Z

Recording some of the offline conversation: Initially the inheritance and metaclasses was for allowing code reuse of thread local code easily for multiple modules. This code was a bit complicated to understand so we moved the threading.local calls to module where it was needed instead of using inheritance and metaclasses for threadlocal. For adding backward compatibility, we had to make default_ctx and equivalents in other modules class properties. This required adding logic in base.py for the same. This included adding a metaclass and since metaclass syntax is not the same between python2 and python3 needed to use six. We cannot add six as a dependency, because that restricts users more, and thus we copied the metaclass specific code from six into base.py.

* Change to simpler implementation * Add property * Remove pdb * Add support for setter and getter * fix issues * Add warnings * Add thread local unittest and tlocal race condition * Fix pylint * Use current_context instead of _default_ctx * Use current_context * Fix race condition * Fix thread local test * Change to current_context

cclauss · 2018-07-12T16:18:54Z

We cannot add six as a dependency

@szha Is this true? I find numerous instances where Python2-only builtins like basestring, long, reload, unicode, xrange are used. Do we need try/except blocks to define each of these or can we use six.moves?

szha · 2018-07-12T16:47:07Z

@cclauss I'd probably describe this as "six is not necessary", as we already handle most of the import compatibility in base.py where we use the python version to decide the definition, and we've been careful about not using version-specific features in our code base (e.g. we have both py2 and py3 in CI). Thus, adding six would mostly just benefit code that's outside of our code base, such as the examples, in which case the dependency is not managed by mxnet and authors are free to choose.

anirudh2290 requested a review from szha as a code owner May 7, 2018 02:22

szha reviewed May 7, 2018

View reviewed changes

piiswrong reviewed May 7, 2018

View reviewed changes

piiswrong suggested changes May 7, 2018

View reviewed changes

anirudh2290 added 10 commits May 8, 2018 07:45

Change to simpler implementation

1a8dda6

Add property

9c5c903

Merge branch 'master' of https://github.com/dmlc/mxnet into fix_globa…

d4cc2af

…l_ctx

Remove pdb

4623ce5

Add support for setter and getter

d8b1b2f

fix issues

40fc6bb

Add warnings

6b79d89

Add thread local unittest and tlocal race condition

df057e2

Fix pylint

3d6501f

Merge branch 'master' of https://github.com/dmlc/mxnet into fix_globa…

b4996ac

…l_ctx

anirudh2290 force-pushed the fix_global_ctx branch from 91c1737 to b4996ac Compare May 10, 2018 01:35

piiswrong suggested changes May 10, 2018

View reviewed changes

anirudh2290 added 5 commits May 10, 2018 06:57

Use current_context instead of _default_ctx

57af178

Use current_context

d12e4a7

Fix race condition

1a91314

Fix thread local test

c31cf59

Change to current_context

0af608b

anirudh2290 changed the title ~~[WIP] Change class variables to thread local variables~~ Change class variables to thread local variables May 11, 2018

piiswrong merged commit 86ee3e1 into apache:master May 11, 2018

eric-haibin-lin mentioned this pull request May 13, 2018

[Python2] Occasional and random Illegal Memory Access Error | Cached Op Input Context Error #10240

Closed

This was referenced Jul 12, 2018

Some Python 3 fixes in ./example #11671

Merged

Some Python 3 fixes in ./tools #11672

Merged

Some Python 3 fixes in ./tools again #11757

Merged

MinZhaoLin mentioned this pull request Aug 24, 2018

Asynchronous Issue on CustomOP and mxnet.image.ImageDetIter #9920

Closed

wkcn mentioned this pull request Mar 6, 2019

Namescope is None when hybridize in multi-threading environment. AttributeError: 'NoneType' object has no attribute '__exit__' #13199

Open

Conversation

anirudh2290 commented May 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Essentials

Changes

Comments

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anirudh2290 May 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piiswrong commented May 10, 2018

Uh oh!

anirudh2290 commented May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cclauss commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szha commented Jul 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

anirudh2290 commented May 7, 2018 •

edited

Loading

anirudh2290 May 7, 2018 •

edited

Loading

anirudh2290 commented May 11, 2018 •

edited

Loading

cclauss commented Jul 12, 2018 •

edited

Loading