Refine runtime feature discovery python API and add documentation to … #14130

larroy · 2019-02-12T10:40:21Z

…Python API docs

Description

see title

@aaronmarkham

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

Decode feature name to string since it comes as a buffer
Add repr as beautifier

iblislin · 2019-02-12T14:47:47Z

docs/api/python/libinfo/libinfo.md

+ ✔ DIST_KVSTORE,
+ ✔ CXX14,
+ ✔ SIGNAL_HANDLER,
+ ✖ DEBUG]


❤️ this

I copied from you :-)

ankkhedia · 2019-02-12T20:22:03Z

@larroy Thanks for your contribution!
@mxnet-label-bot add [pr-awaiting-review, Doc]

szha · 2019-02-12T23:42:57Z

python/mxnet/runtime.py

+    feature_dict = {f.name: f.enabled for f in libinfo_features()}
+    if tocheck not in feature_dict:
+        raise RuntimeError("Feature '{}' is unknown, known features are: {}".format(tocheck, list(feature_dict.keys())))
+    return feature_dict[tocheck]


if a dictionary is easier to operate, should the libinfo_features return a dictionary?

I prefer to keep ordering as in a list. Going from list to dict is oneliner. Also we don't get a dict from C.

is the order information useful? I always thought of it as a set. also, by defining your own data structure class, you can define more pythonic behaviors such as __contains__ and __getitem__ to make it easier for feature check.

They are grouped if you see the enum definition as some features are related, semantically is a set as you describe. For using contains as you propose we would have to wrap the list in a containing class. I don't see a lot of value on complicating this further TBH. Could you provide an example of what you have in mind if you feel strongly about it?

I don't feel stongly about the change. What I had in mind was something like:

>>> features = mx.runtime.libinfo_features() >>> 'NCCL' in features True >>> 'nccl' in features True >>> nccl_idx = 2; >>> nccl_idx in features True >>> features['NCCL'] ✔ NCCL >>> features['NCCL'].index 2 >>> features['NCCL'].enabled True

I see, that looks nice. On the upside repeated calls are more efficient than my proposal. On the downside I think the primary case which is to check if a feature is enabled is less ergonomic due to the syntactic sugar. I think a simple function with a single arg is always clearer. A good point is that it should be case insensitive. I'm a bit torn. I will refine it a bit more.

Just curious: is the index information useful to user? Why do we even need to expose index?

@apeforest I just removed it.

apeforest · 2019-02-13T18:12:39Z

tests/python/unittest/test_runtime.py

@@ -29,6 +29,17 @@ def test_libinfo_features():
    ok_(type(features) is list)
    ok_(len(features) > 0)

+def test_is_enabled():


Can this test run in a docker image with CUDA on the machine without GPU?

Not sure I get what's the intention.

larroy · 2019-02-14T00:28:46Z

I made the suggested changed, please have another look and merge if you are good with it, thanks.

docs/api/python/libinfo/libinfo.md

…Python API docs

apache#14130) * Refine runtime feature discovery python API and add documentation to Python API docs * Fix lint * Provide is_enabled method to check if feature is present from string * Refine docs, add is_enabled * Fix encoding * Fix doc * Address CR suggestions * remove index as per CR suggestion * Fix lint * runtime * Fix doc * Add license

larroy · 2019-02-19T13:40:14Z

Thanks for the merge and the review.

apache#14130) * Refine runtime feature discovery python API and add documentation to Python API docs * Fix lint * Provide is_enabled method to check if feature is present from string * Refine docs, add is_enabled * Fix encoding * Fix doc * Address CR suggestions * remove index as per CR suggestion * Fix lint * runtime * Fix doc * Add license

larroy requested a review from szha as a code owner February 12, 2019 10:40

larroy force-pushed the feature_discovery branch 2 times, most recently from 5da0748 to 7a39b9a Compare February 12, 2019 14:31

iblislin reviewed Feb 12, 2019

View reviewed changes

larroy force-pushed the feature_discovery branch from a74d87f to 9b1463f Compare February 12, 2019 18:07

marcoabreu added Doc pr-awaiting-review PR is waiting for code review labels Feb 12, 2019

larroy force-pushed the feature_discovery branch from 9b1463f to 0366b48 Compare February 12, 2019 23:36

szha reviewed Feb 12, 2019

View reviewed changes

apeforest reviewed Feb 13, 2019

View reviewed changes

larroy force-pushed the feature_discovery branch from cb99c05 to f09ed43 Compare February 14, 2019 00:19

larroy requested a review from anirudh2290 as a code owner February 14, 2019 00:25

szha reviewed Feb 14, 2019

View reviewed changes

docs/api/python/libinfo/libinfo.md Outdated Show resolved Hide resolved

larroy force-pushed the feature_discovery branch from 8f66288 to ede3f71 Compare February 14, 2019 12:53

Pedro Larroy and others added 12 commits February 15, 2019 15:46

Refine runtime feature discovery python API and add documentation to …

b84e22f

…Python API docs

Fix lint

07c0ef0

Provide is_enabled method to check if feature is present from string

b9b4fc9

Refine docs, add is_enabled

aa097c1

Fix encoding

dcf6bd9

Fix doc

3927fc2

Address CR suggestions

1d93f79

remove index as per CR suggestion

834906a

Fix lint

e7613f6

runtime

8a46442

Fix doc

cce62f1

Add license

dd432b7

larroy force-pushed the feature_discovery branch from 6cb7a82 to dd432b7 Compare February 15, 2019 14:46

szha merged commit 3a6fe22 into apache:master Feb 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refine runtime feature discovery python API and add documentation to … #14130

Refine runtime feature discovery python API and add documentation to … #14130

larroy commented Feb 12, 2019 •

edited

Loading

iblislin Feb 12, 2019

larroy Feb 12, 2019

ankkhedia commented Feb 12, 2019

szha Feb 12, 2019

larroy Feb 12, 2019 •

edited

Loading

szha Feb 13, 2019

larroy Feb 13, 2019

szha Feb 13, 2019

larroy Feb 13, 2019 •

edited

Loading

apeforest Feb 13, 2019 •

edited

Loading

larroy Feb 14, 2019

apeforest Feb 13, 2019

larroy Feb 14, 2019

larroy commented Feb 14, 2019

larroy commented Feb 19, 2019

Refine runtime feature discovery python API and add documentation to … #14130

Refine runtime feature discovery python API and add documentation to … #14130

Conversation

larroy commented Feb 12, 2019 • edited Loading

Description

Checklist

Essentials

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankkhedia commented Feb 12, 2019

Choose a reason for hiding this comment

larroy Feb 12, 2019 • 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

larroy Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

apeforest Feb 13, 2019 • 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

larroy commented Feb 14, 2019

larroy commented Feb 19, 2019

larroy commented Feb 12, 2019 •

edited

Loading

larroy Feb 12, 2019 •

edited

Loading

larroy Feb 13, 2019 •

edited

Loading

apeforest Feb 13, 2019 •

edited

Loading