[MXNET-360]auto convert str to bytes in img.imdecode when py3 #10697

yajiedesign · 2018-04-26T07:51:36Z

Description

auto convert str to bytes in img.imdecode when py3

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

Comments

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

piiswrong · 2018-04-26T17:39:15Z

python/mxnet/image/image.py

@@ -132,6 +134,9 @@ def imdecode(buf, *args, **kwargs):
    <NDArray 224x224x3 @cpu(0)>
    """
    if not isinstance(buf, nd.NDArray):
+        if sys.version_info[0] == 3:
+            if isinstance(buf, str):
+                buf = bytes(buf, "ascii")


could you add a test?
also we have mx.base.py_str. Would that work?

mx.base.py_str is 'utf8' encoding,I'm not sure it can work.

how about using buffer?

This case really shouldn't happen, because in this case the API is underspecified.

String is always tied to an encoding, and if the string comes from the user, it could have been decoded with any encoding/charset, so the 'ascii' assumption can easily be wrong.

@szha or I change it into a friendly error message. prompt user convet it himself.

piiswrong · 2018-04-26T17:39:55Z

Is this going to cause error if no converting is done?

yajiedesign · 2018-04-27T00:46:13Z

@piiswrong np.frombuffer receive buffer like param,but in python3 str not is buffer like.will trigger TypeError.

piiswrong · 2018-04-30T17:09:33Z

I can merge this work around first. But I think we should have a mx.nd.frombuffer that uses CAPI directly.

BTW why would buf be str in python3? open('xxx', 'b').read() produces bytes in py3 right?

yajiedesign · 2018-04-30T22:52:34Z

This conversion is used to be compatible with python2,If someone saves data with python2 and reads it with python3, it will go wrong.

chinakook · 2018-05-02T14:01:01Z

It's a bug of numpy -- numpy/numpy#8933

lupesko · 2018-08-06T20:36:18Z

@piiswrong are you still planning to merge this?

lupesko · 2018-08-21T06:21:55Z

@piiswrong @zhreshold bouncing for a review. thanks!

zhreshold · 2018-08-21T22:08:23Z

It is still relevant now?

yajiedesign · 2018-09-03T11:55:32Z

@zhreshold Numpy has not been fixed. This examination should be useful.

lupesko · 2018-09-14T08:12:50Z

Adding @sandeep-krishnamurthy @Roshrini @vandanavk for review.
@mxnet-label-bot [pr-awaiting-review]

lupesko

Some minor wording nit.

lupesko · 2018-09-14T08:15:01Z

python/mxnet/image/image.py

@@ -133,6 +135,9 @@ def imdecode(buf, *args, **kwargs):
    <NDArray 224x224x3 @cpu(0)>
    """
    if not isinstance(buf, nd.NDArray):
+        if sys.version_info[0] == 3 and not isinstance(buf, (bytes, np.ndarray)):
+            raise ValueError('buf must bytes or numpy.ndarray,'


nit: suggest rephrasing a bit to "buf must be of type bytes or numpy.ndarray"

lupesko · 2018-09-14T08:15:57Z

python/mxnet/image/image.py

@@ -133,6 +135,9 @@ def imdecode(buf, *args, **kwargs):
    <NDArray 224x224x3 @cpu(0)>
    """
    if not isinstance(buf, nd.NDArray):
+        if sys.version_info[0] == 3 and not isinstance(buf, (bytes, np.ndarray)):
+            raise ValueError('buf must bytes or numpy.ndarray,'
+                             'if you input str,please convert it to bytes')


nit: suggest rephrase: "if you would like to input type str, please convert to bytes"

yajiedesign · 2018-09-14T12:48:33Z

@lupesko done

sandeep-krishnamurthy

LGTM.
This is still relevant edge case numpy/numpy#8933 as pointed out by @chinakook

sandeep-krishnamurthy · 2018-09-14T17:54:08Z

@szha @piiswrong - ping for any other concerns.

sandeep-krishnamurthy · 2018-09-21T04:30:38Z

@zhreshold - This is good to go, if it looks fine for you.

yajiedesign requested a review from szha as a code owner April 26, 2018 07:51

yajiedesign force-pushed the fix_imdecode branch from a872b87 to 8f8e104 Compare April 26, 2018 08:04

piiswrong reviewed Apr 26, 2018

View reviewed changes

yajiedesign force-pushed the fix_imdecode branch from 8f8e104 to 5234b10 Compare April 27, 2018 00:36

szha requested review from zhreshold and removed request for szha May 21, 2018 19:53

change to friendly error message

50e6b02

yajiedesign force-pushed the fix_imdecode branch from 6f46e0d to 50e6b02 Compare July 1, 2018 15:02

yajiedesign added 3 commits July 2, 2018 11:12

fix lint

8ba6fb5

fix lint

a419ecd

fix lint

96a6974

marcoabreu added the pr-awaiting-review PR is waiting for code review label Sep 14, 2018

lupesko suggested changes Sep 14, 2018

View reviewed changes

update raise message

8bd8a5f

sandeep-krishnamurthy approved these changes Sep 14, 2018

View reviewed changes

sandeep-krishnamurthy added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-awaiting-review PR is waiting for code review labels Sep 14, 2018

zhreshold merged commit d4991a0 into apache:master Sep 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MXNET-360]auto convert str to bytes in img.imdecode when py3 #10697

[MXNET-360]auto convert str to bytes in img.imdecode when py3 #10697

yajiedesign commented Apr 26, 2018 •

edited

Loading

piiswrong Apr 26, 2018

yajiedesign Apr 27, 2018

piiswrong Apr 30, 2018 •

edited

Loading

szha May 2, 2018

yajiedesign May 2, 2018 •

edited

Loading

szha May 2, 2018

piiswrong commented Apr 26, 2018

yajiedesign commented Apr 27, 2018 •

edited

Loading

piiswrong commented Apr 30, 2018

yajiedesign commented Apr 30, 2018

chinakook commented May 2, 2018

lupesko commented Aug 6, 2018

lupesko commented Aug 21, 2018

zhreshold commented Aug 21, 2018

yajiedesign commented Sep 3, 2018

lupesko commented Sep 14, 2018

lupesko left a comment

lupesko Sep 14, 2018

lupesko Sep 14, 2018

yajiedesign commented Sep 14, 2018

sandeep-krishnamurthy left a comment

sandeep-krishnamurthy commented Sep 14, 2018

sandeep-krishnamurthy commented Sep 21, 2018

[MXNET-360]auto convert str to bytes in img.imdecode when py3 #10697

[MXNET-360]auto convert str to bytes in img.imdecode when py3 #10697

Conversation

yajiedesign commented Apr 26, 2018 • edited Loading

Description

Checklist

Essentials

Changes

Comments

piiswrong Apr 26, 2018

Choose a reason for hiding this comment

yajiedesign Apr 27, 2018

Choose a reason for hiding this comment

piiswrong Apr 30, 2018 • edited Loading

Choose a reason for hiding this comment

szha May 2, 2018

Choose a reason for hiding this comment

yajiedesign May 2, 2018 • edited Loading

Choose a reason for hiding this comment

szha May 2, 2018

Choose a reason for hiding this comment

piiswrong commented Apr 26, 2018

yajiedesign commented Apr 27, 2018 • edited Loading

piiswrong commented Apr 30, 2018

yajiedesign commented Apr 30, 2018

chinakook commented May 2, 2018

lupesko commented Aug 6, 2018

lupesko commented Aug 21, 2018

zhreshold commented Aug 21, 2018

yajiedesign commented Sep 3, 2018

lupesko commented Sep 14, 2018

lupesko left a comment

Choose a reason for hiding this comment

lupesko Sep 14, 2018

Choose a reason for hiding this comment

lupesko Sep 14, 2018

Choose a reason for hiding this comment

yajiedesign commented Sep 14, 2018

sandeep-krishnamurthy left a comment

Choose a reason for hiding this comment

sandeep-krishnamurthy commented Sep 14, 2018

sandeep-krishnamurthy commented Sep 21, 2018

yajiedesign commented Apr 26, 2018 •

edited

Loading

piiswrong Apr 30, 2018 •

edited

Loading

yajiedesign May 2, 2018 •

edited

Loading

yajiedesign commented Apr 27, 2018 •

edited

Loading