adding error message when attempting to use Large tensor with linalg_syevd #18807

access2rohit · 2020-07-28T11:50:36Z

Description

This PR adds error message when very large inputs(>2^32-1) are passed to linear algebra operator 'syevd'

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

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)
To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

mxnet-bot · 2020-07-28T11:50:40Z

Hey @access2rohit , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:

To trigger all jobs: @mxnet-bot run ci [all]
To trigger specific jobs: @mxnet-bot run ci [job1, job2]

CI supported jobs: [website, miscellaneous, centos-cpu, centos-gpu, windows-cpu, windows-gpu, unix-gpu, clang, sanity, edge, unix-cpu]

Note:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.

sandeep-krishnamurthy · 2020-07-28T15:29:59Z

3rdparty/mshadow/mshadow/base.h

@@ -336,6 +336,8 @@ const float kPi = 3.1415926f;
  typedef int32_t index_t;
 #endif

+const index_t kInt32Limit = (int64_t{1} << 31) - 1;


Can we add a comment to make it clear where is this being used.

what's the advantage of manually defining this over using INT_MAX in <climits?

@Zha0q1 good suggestion !

ChaiBapchya

LGTM
Do you think adding a test with assert_exception in test_large_array.py
For example

from mxnet.test_utils import assert_exception
def test_linalg_syevd():
    input = mx.nd.array(LARGE_X,SMALL_Y)
    assert_exception(mx.nd.linalg(data), MXNetError)

access2rohit · 2020-07-28T18:19:06Z

LGTM
Do you think adding a test with assert_exception in test_large_array.py
For example
from mxnet.test_utils import assert_exception
def test_linalg_syevd():
    input = mx.nd.array(LARGE_X,SMALL_Y)
    assert_exception(mx.nd.linalg(data), MXNetError)

Thats why WIP

access2rohit · 2020-07-28T20:17:17Z

(pytest) ubuntu@ip-172-31-90-243 ~/workspace/incubator-mxnet (err_msg) $ python -m pytest -s --exitfirst --verbose tests/nightly/test_large_array.py::test_linalg_errors
=============================================================================================== test session starts ================================================================================================
platform linux -- Python 3.6.10, pytest-5.3.5, py-1.8.2, pluggy-0.13.1 -- /home/ubuntu/anaconda3/envs/pytest/bin/python
cachedir: .pytest_cache
rootdir: /home/ubuntu/workspace/incubator-mxnet
collected 1 item

tests/nightly/test_large_array.py::test_linalg_errors PASSED

================================================================================================= warnings summary =================================================================================================
/home/ubuntu/anaconda3/envs/pytest/lib/python3.6/site-packages/nose/importer.py:12
  /home/ubuntu/anaconda3/envs/pytest/lib/python3.6/site-packages/nose/importer.py:12: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
    from imp import find_module, load_module, acquire_lock, release_lock

-- Docs: https://docs.pytest.org/en/latest/warnings.html

access2rohit · 2020-07-28T21:31:29Z

Test Run

(pytest) ubuntu@ip-172-31-90-243 ~/workspace/incubator-mxnet (err_msg) $ python -m pytest -s --exitfirst --verbose tests/nightly/test_large_array.py::test_linalg_errors
=============================================================================================== test session starts ================================================================================================
platform linux -- Python 3.6.10, pytest-5.3.5, py-1.8.2, pluggy-0.13.1 -- /home/ubuntu/anaconda3/envs/pytest/bin/python
cachedir: .pytest_cache
rootdir: /home/ubuntu/workspace/incubator-mxnet
collected 1 item

tests/nightly/test_large_array.py::test_linalg_errors PASSED

================================================================================================= warnings summary =================================================================================================
/home/ubuntu/anaconda3/envs/pytest/lib/python3.6/site-packages/nose/importer.py:12
  /home/ubuntu/anaconda3/envs/pytest/lib/python3.6/site-packages/nose/importer.py:12: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
    from imp import find_module, load_module, acquire_lock, release_lock

-- Docs: https://docs.pytest.org/en/latest/warnings.html
========================================================================================== 1 passed, 1 warning in 28.18s ===========================================================================================

Zha0q1

left some comments on minor changes. Otherwise looking good!

Zha0q1 · 2020-07-28T21:30:42Z

src/operator/tensor/la_op.h

@@ -470,11 +470,14 @@ inline bool DetType(const nnvm::NodeAttrs& attrs,
 inline bool LaEigFactShape(const nnvm::NodeAttrs& attrs,
                           mxnet::ShapeVector* in_attrs,
                           mxnet::ShapeVector* out_attrs) {
+  using namespace mshadow;


this can be removed then.

Zha0q1 · 2020-07-28T21:31:25Z

src/operator/tensor/la_op.h

  CHECK_EQ(in_attrs->size(), 1);
  CHECK_EQ(out_attrs->size(), 2);
  const mxnet::TShape& in_a = (*in_attrs)[0];
  const mxnet::TShape& out_u = (*out_attrs)[0];
  const mxnet::TShape& out_l = (*out_attrs)[1];
+  CHECK_LE(in_a.Size(), INT_MAX)


did you include climit ?

is included in base.h

It's better to be explicit about your dependencies.

"Avoid surprises. Avoid having to change #includes if an #included header changes. Avoid accidentally becoming dependent on implementation details and logically separate entities included in a header."

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rs-implicit

tests/nightly/test_large_array.py

+        A = get_identity_mat(LARGE_SQ_X)
+        for i in range(LARGE_SQ_X):
+            A[i,i] = 1
+        assertRaises(MXNetError, mx.nd.linalg.syevd, A)


ChaiBapchya

Thanks. LGTM!

access2rohit · 2020-07-29T05:55:07Z

@mxnet-bot run ci [unix-gpu]

mxnet-bot · 2020-07-29T05:55:13Z

Jenkins CI successfully triggered : [unix-gpu]

…syevd

leezu · 2020-07-29T21:29:43Z

@mxnet-bot run ci [clang]

mxnet-bot · 2020-07-29T21:29:50Z

Jenkins CI successfully triggered : [clang]

sandeep-krishnamurthy reviewed Jul 28, 2020

View reviewed changes

josephevans approved these changes Jul 28, 2020

View reviewed changes

ChaiBapchya reviewed Jul 28, 2020

View reviewed changes

access2rohit force-pushed the err_msg branch from 79f11fc to 70b385e Compare July 28, 2020 20:17

access2rohit changed the title ~~[WIP]adding error message when attempting to use Large tensor with linalg_syevd~~ adding error message when attempting to use Large tensor with linalg_syevd Jul 28, 2020

access2rohit force-pushed the err_msg branch 2 times, most recently from ed5a883 to 6411cee Compare July 28, 2020 21:28

Zha0q1 reviewed Jul 28, 2020

View reviewed changes

tests/nightly/test_large_array.py

A = get_identity_mat(LARGE_SQ_X)

for i in range(LARGE_SQ_X):

A[i,i] = 1

assertRaises(MXNetError, mx.nd.linalg.syevd, A)

This comment was marked as outdated.

Sign in to view

access2rohit force-pushed the err_msg branch from 6411cee to 61181ce Compare July 28, 2020 23:54

Zha0q1 approved these changes Jul 28, 2020

View reviewed changes

access2rohit force-pushed the err_msg branch from 61181ce to 45baa34 Compare July 29, 2020 00:17

ChaiBapchya approved these changes Jul 29, 2020

View reviewed changes

adding error message when attempting to use Large tensor with linalg_…

ec5f66a

…syevd

access2rohit force-pushed the err_msg branch from 45baa34 to ec5f66a Compare July 29, 2020 14:50

leezu merged commit ca6bcf3 into apache:v1.x Jul 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding error message when attempting to use Large tensor with linalg_syevd #18807

adding error message when attempting to use Large tensor with linalg_syevd #18807

access2rohit commented Jul 28, 2020 •

edited

Loading

mxnet-bot commented Jul 28, 2020

sandeep-krishnamurthy Jul 28, 2020

Zha0q1 Jul 28, 2020 •

edited

Loading

access2rohit Jul 28, 2020

ChaiBapchya left a comment

access2rohit commented Jul 28, 2020

access2rohit commented Jul 28, 2020 •

edited

Loading

access2rohit commented Jul 28, 2020

Zha0q1 left a comment

Zha0q1 Jul 28, 2020

Zha0q1 Jul 28, 2020

access2rohit Jul 28, 2020

leezu Jul 29, 2020

This comment was marked as outdated.

ChaiBapchya left a comment

access2rohit commented Jul 29, 2020

mxnet-bot commented Jul 29, 2020

leezu commented Jul 29, 2020

mxnet-bot commented Jul 29, 2020

adding error message when attempting to use Large tensor with linalg_syevd #18807

adding error message when attempting to use Large tensor with linalg_syevd #18807

Conversation

access2rohit commented Jul 28, 2020 • edited Loading

Description

Checklist

Essentials

mxnet-bot commented Jul 28, 2020

sandeep-krishnamurthy Jul 28, 2020

Choose a reason for hiding this comment

Zha0q1 Jul 28, 2020 • edited Loading

Choose a reason for hiding this comment

access2rohit Jul 28, 2020

Choose a reason for hiding this comment

ChaiBapchya left a comment

Choose a reason for hiding this comment

access2rohit commented Jul 28, 2020

access2rohit commented Jul 28, 2020 • edited Loading

access2rohit commented Jul 28, 2020

Test Run

Zha0q1 left a comment

Choose a reason for hiding this comment

Zha0q1 Jul 28, 2020

Choose a reason for hiding this comment

Zha0q1 Jul 28, 2020

Choose a reason for hiding this comment

access2rohit Jul 28, 2020

Choose a reason for hiding this comment

leezu Jul 29, 2020

Choose a reason for hiding this comment

This comment was marked as outdated.

ChaiBapchya left a comment

Choose a reason for hiding this comment

access2rohit commented Jul 29, 2020

mxnet-bot commented Jul 29, 2020

leezu commented Jul 29, 2020

mxnet-bot commented Jul 29, 2020

access2rohit commented Jul 28, 2020 •

edited

Loading

Zha0q1 Jul 28, 2020 •

edited

Loading

access2rohit commented Jul 28, 2020 •

edited

Loading