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

Fixes for wine detection tutorial #13886

Merged
merged 2 commits into from
Apr 29, 2019
Merged

Conversation

larroy
Copy link
Contributor

@larroy larroy commented Jan 15, 2019

Description

see tittle

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

@larroy larroy requested a review from szha as a code owner January 15, 2019 15:33
@stu1130
Copy link
Contributor

stu1130 commented Jan 15, 2019

@mxnet-label-bot add [pr-awaiting-review]
Thanks for your great work @larroy

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label Jan 15, 2019
@larroy
Copy link
Contributor Author

larroy commented Jan 15, 2019

#9094

@larroy larroy mentioned this pull request Jan 15, 2019
Copy link
Contributor

@jlcontreras jlcontreras left a comment

Choose a reason for hiding this comment

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

LGTM

@vandanavk
Copy link
Contributor

@aaronmarkham for review/merge

Copy link
Contributor

@aaronmarkham aaronmarkham left a comment

Choose a reason for hiding this comment

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

I'm not going to hold this up with my testing as my hardware is Pi Model B v2 and only has 512MB of RAM. I keep getting stuck trying into install numpy. The changes look fine, but at the moment I can't validate the functionality of the tutorial.
One thing to note though - the instructions tell you to follow the install page's setup, but those tell you to create a Python 3 env, whereas this tutorial is 2.7. Minor detail, but maybe this could be fixed.

Here's the flow I started (but stopped at the install step as it is hanging):
Format your SD card with SD Card Formatter.
Download, unzip, then copy contents of the NOOBS (network install) zip file to SD card.
Boot up the Pi and follow the prompts to install the headless Raspbian (smaller and might help with memory issues).
Login to the pi with user pi and password raspberry.
Install pip with:

sudo apt-get install python-pip

Install virtualenv with:

sudo pip install virtualenv

Create a Python 2.7 environment for MXNet with:

virtualenv -p `which python` mxnet_py27

Download the mxnet wheel with:

wget https://mxnet-public.s3.amazonaws.com/install/raspbian/mxnet-1.5.0-py2.py3-none-any.whl

Install mxnet with:

pip install mxnet-1.5.0-py2.py3-none-any.whl

That wheel is something I created. It would be nice if we hosted some tagged versions somewhere, so people can grab a wheel with v1.3.1 or the latest RC.
Also, I don't see that this tutorial has a link from the tutorial's index page. So how does one find this tutorial?

Copy link
Contributor

@aaronmarkham aaronmarkham left a comment

Choose a reason for hiding this comment

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

I still haven't seen this work yet, but I can tell you that the earlier step where you copy the model params file is incorrect. The file listed isn't in the tar. Also the dependencies setup isn't complete.

I have my notes on how to get MXNet running on a Pi 3 with Stretch here:
https://gist.github.com/aaronmarkham/b3d0c715abca92ccc4b79ed0f19d1916

A Pi 2 doesn't work with MXNet (using the cross-compiled wheel) - it core dumps.

One further issue with this tutorial is that it requires OpenCV2 to be installed separately. This would require a build from source as there doesn't seem to be a package available for Stretch. Then I ran out of disk space on my 8GB card before I could go further. This could be avoided though since the image processing steps are already part of MXNet's port of OpenCV operators. We don't need OpenCV 2 here.

I'm not going to approve or request changes. If you know this update works on your test devices that say so, and we'll merge this update. However, I think that this tutorial needs to be modernized to use the latest MXNet features for handling images. And we should also have the Install page updated so that it has the appropriate dependencies.

Additionally, I found it a pain to have to compile on Ubuntu, and I really wish that we offered wheels prepped for whatever Pi & MXNet versions we can support.

@aaronmarkham
Copy link
Contributor

I put the edits for MXNet installation on Raspberry Pi in this PR: #14172

@aaronmarkham
Copy link
Contributor

I also just fixed the conflict from the new license header.

Copy link
Contributor

@ankkhedia ankkhedia left a comment

Choose a reason for hiding this comment

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

LGTM

@ankkhedia
Copy link
Contributor

@larroy Could you look into comments by @aaronmarkham

@aaronmarkham
Copy link
Contributor

I updated the Pi instructions here: #14172
So @larroy, if you know the tutorial works, then LMK and I'll merge this update. I can raise my issues with the preprocessing part of the tutorial in a separate issue.

@@ -100,13 +101,14 @@ with open('synset.txt', 'r') as f:
synsets = [l.rstrip() for l in f]

# Load the network parameters
sym, arg_params, aux_params = mx.model.load_checkpoint('Inception_BN', 0)
sym, arg_params, aux_params = mx.model.load_checkpoint('Inception-BN', 126)
Copy link
Member

Choose a reason for hiding this comment

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

magic number?

Copy link
Member

Choose a reason for hiding this comment

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

It is the number of epoch.

Copy link
Member

Choose a reason for hiding this comment

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

Right, what I mean is this probably shouldn't be hard-coded?

Copy link
Member

Choose a reason for hiding this comment

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

Agree.

Copy link
Contributor Author

@larroy larroy Apr 19, 2019

Choose a reason for hiding this comment

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

Maybe, but if you follow the instructions, the model donloaded has that epoch for the checkpoint. What do you suggest instead?

@vandanavk
Copy link
Contributor

@mxnet-label-bot update [Example, pr-awaiting-response]

@marcoabreu marcoabreu added Example pr-awaiting-response PR is reviewed and waiting for contributor to respond and removed pr-awaiting-review PR is waiting for code review labels Mar 4, 2019
@karan6181
Copy link
Contributor

@larroy Could you please address the review comments? Thanks!

Copy link
Member

@wkcn wkcn left a comment

Choose a reason for hiding this comment

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

Thank you for the fix! LGTM

@abhinavs95
Copy link
Contributor

@larroy Could you please give an update on the PR? Thanks

@piyushghai
Copy link
Contributor

@larroy Gentle ping...

@Roshrini
Copy link
Member

@aaronmarkham @larroy Is this PR good to go?

@aaronmarkham
Copy link
Contributor

@larroy has an unresolved comment here. Once that's done and he's sure it works, I'm ok with the PR.

@larroy
Copy link
Contributor Author

larroy commented Apr 22, 2019

@aaronmarkham I responded to the comment.

@larroy
Copy link
Contributor Author

larroy commented Apr 22, 2019

@mxnet-label-bot remove [pr-awaiting-response]

@marcoabreu marcoabreu removed the pr-awaiting-response PR is reviewed and waiting for contributor to respond label Apr 22, 2019
@larroy
Copy link
Contributor Author

larroy commented Apr 22, 2019

@mxnet-label-bot add [pr-awaiting-review]

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label Apr 22, 2019
Copy link
Member

@wkcn wkcn left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@aaronmarkham aaronmarkham merged commit 1af29e9 into apache:master Apr 29, 2019
access2rohit pushed a commit to access2rohit/incubator-mxnet that referenced this pull request May 14, 2019
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Example pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.