search

apache / mxnet

Lightweight, Portable, Flexible Distributed/Mobile Deep Learning with Dynamic, Mutation-aware Dataflow Dep Scheduler; for Python, R, Julia, Scala, Go, Javascript and more
https://mxnet.apache.org
Apache License 2.0
20.78k stars 6.79k forks source link

Lint errors in examples #12205

Open vandanavk opened 6 years ago

vandanavk commented 6 years ago

Description

pylint scan on the files in the example folders, gives a number of errors - documented here https://gist.github.com/vandanavk/35e1b5b0a5b6c5654ed78bd831dac442

Environment info (Required)

Package used (Python/R/Scala/Julia): Python 3

Build info (Required if built from source)

Compiler (gcc/clang/mingw/visual studio):

MXNet commit hash: ab92fd80b10805fd103217df0569513289db4625

Build config: (Paste the content of config.mk, or the build command.)

Error Message:

https://gist.github.com/vandanavk/35e1b5b0a5b6c5654ed78bd831dac442

Minimum reproducible example

(If you are using your own code, please provide a short script that reproduces the error. Otherwise, please provide link to the existing example.)

Steps to reproduce

import os

for (dirpath, dirnames, filenames) in os.walk('example'):
    os.system('pylint --rcfile=ci/other/pylintrc --ignore-patterns=".*\.so$$,.*\.dll$$,.*\.dylib$$" '
              '--msg-template="{path}({line}): [{msg_id} {obj}] {msg}" '+dirpath+'/*.py > pylint_output.txt')

or just

pylint --rcfile=ci/other/pylintrc --ignore-patterns="..so$$,..dll$$,..dylib$$" --msg-template="{path}({line}): [{msg_id} {obj}] {msg}" path_to_target_folder/\.py

What have you tried to solve it?

Undefined-name errors have been fixed. These errors are the ones with the default pylintrc.

vandanavk commented 6 years ago

@mxnet-label-bot Please add labels [Example, CI]

srochel commented 6 years ago

@mxnet-label-bot: ["Good First Issue"]

srochel commented 6 years ago

@mxnet-label-bot [Good First Issue]

cchung100m commented 6 years ago

Hi @vandanavk ,

I'm interested in working on this if no one else has signed up. However this is the first time I'm contributing to mxnet so I may need some hand-holding 😃

By the way, I see lots of Missing class/function docstring, I'd like to know that how to add docstring appropriately, for example, where can I find some references, any suggestions?

Many thanks,

vandanavk commented 6 years ago

@cchung100m Welcome! :) It's great that you'd like to work on this and contribute to MXNet.

Here's an example of a class that has docstrings. example/ctc/captcha_generator.py You could write a class description with attribute information for the classes/functions which show an error. Then execute pylint on that file to confirm that the error is fixed.

Please feel free to reach out for further information/help on this.

cchung100m commented 5 years ago

Hi @vandanavk ,

Sorry for the late reply.

I have been working on this issue very hard in the past month, and I solved most reported errors. Now that there are just a few negligible warnings left, I would appreciate your comments and suggestions :)

Best wishes for a Happy New Year! Neo

environment info:

python 3.5.1 pylint 2.1.1

gives the result - documented here

https://gist.github.com/cchung100m/ff41d20ada6ab9efd6e8d7ad3ec5b3e8

the updated source code

https://github.com/cchung100m/incubator-mxnet/tree/issue12205

vandanavk commented 5 years ago

@cchung100m thanks for looking into this! Could you open a PR for review on https://github.com/apache/incubator-mxnet? Everyone in the community will be able to review it and eventually, we can merge it into master.

Since the PR changes 290 files, would it be possible for you to split it and submit for review?

cchung100m commented 5 years ago

Hi @vandanavk,

Thank you for the prompt reply and suggestions.

I'd love to open a PR or PRs for review on https://github.com/apache/incubator-mxnet, however, I don't know how to split the uploading changes into several PRs and I would appreciate if you can give me a hand.

Many thanks,

vandanavk commented 5 years ago

@cchung100m maybe you could split the commit into multiple commits containing changes to 10-15 files each and submit as stacked PRs (ref: https://graysonkoonce.com/stacked-pull-requests-keeping-github-diffs-small/).

tagging @marcoabreu and @sandeep-krishnamurthy for inputs on any alternate/efficient method to submit large commits for review.

cchung100m commented 5 years ago

Hi @vandanavk ,

Thank you for the suggestions.

I split the whole changes into multiple commits(total 20 parts and each commit contains 15 files). However, I cannot stack the PRs as below:

(venv) cch:incubator-mxnet cch$ git show-branch
! [issue12205] solve pylint errors in examples with issue no.12205
 ! [issue12205_2] [issue_12205 - PART1]
  * [issue12205_3] [issue_12205 - PART2]
   ! [issue12205_4] [issue_12205 - PART3]
    ! [issue12205_5] [issue_12205 - PART4]
     ! [issue12205_6] [issue_12205 - PART5]
      ! [issue12205_7] [issue_12205 - PART6]
       ! [issue12205_8] [issue_12205 - PART7]
        ! [issue12205_9] [issue_12205 - PART8]
         ! [issue12205_10] [issue_12205 - PART9]
          ! [issue12205_11] [issue_12205 - PART10]
           ! [issue12205_12] [issue_12205 - PART11]
            ! [issue12205_13] [issue_12205 - PART12]
             ! [issue12205_14] [issue_12205 - PART13]
              ! [issue12205_15] [issue_12205 - PART14]
               ! [issue12205_16] [issue_12205 - PART15]
                ! [issue12205_17] [issue_12205 - PART16]
                 ! [issue12205_18] [issue_12205 - PART17]
                  ! [issue12205_19] [issue_12205 - PART18]
                   ! [issue12205_20] [issue_12205 - PART19]
                    ! [issue12205_21] [issue_12205 - PART20]
                     ! [master] AdamW operator (Fixing Weight Decay Regularization in Adam) (#13728)
----------------------
                    +  [issue12205_21] [issue_12205 - PART20]
                   ++  [issue12205_20] [issue_12205 - PART19]
                  +++  [issue12205_19] [issue_12205 - PART18]
                 ++++  [issue12205_18] [issue_12205 - PART17]
                +++++  [issue12205_17] [issue_12205 - PART16]
               ++++++  [issue12205_16] [issue_12205 - PART15]
              +++++++  [issue12205_15] [issue_12205 - PART14]
             ++++++++  [issue12205_14] [issue_12205 - PART13]
            +++++++++  [issue12205_13] [issue_12205 - PART12]
           ++++++++++  [issue12205_12] [issue_12205 - PART11]
          +++++++++++  [issue12205_11] [issue_12205 - PART10]
         ++++++++++++  [issue12205_10] [issue_12205 - PART9]
        +++++++++++++  [issue12205_9] [issue_12205 - PART8]
       ++++++++++++++  [issue12205_8] [issue_12205 - PART7]
      +++++++++++++++  [issue12205_7] [issue_12205 - PART6]
     ++++++++++++++++  [issue12205_6] [issue_12205 - PART5]
    +++++++++++++++++  [issue12205_5] [issue_12205 - PART4]
   ++++++++++++++++++  [issue12205_4] [issue_12205 - PART3]
  *++++++++++++++++++  [issue12205_3] [issue_12205 - PART2]
 +*++++++++++++++++++  [issue12205_2] [issue_12205 - PART1]
 +*+++++++++++++++++++ [master] AdamW operator (Fixing Weight Decay Regularization in Adam) (#13728)

(venv) cch:incubator-mxnet cch$ hub pull-request -b issue12205_2
Error creating pull request: Unprocessable Entity (HTTP 422)
Invalid value for "base"

Therefore, I open the PRs from Github website, but I am not sure if it is stacked PRs.

https://github.com/apache/incubator-mxnet/pull/13815

https://github.com/apache/incubator-mxnet/pull/13848

I would appreciate if you can give me further suggestions.

Many thanks,

vandanavk commented 5 years ago

@cchung100m this is great. I had a look at #13815, #13848. The changes LGTM. I have requested more reviewers to look into it. In the meantime, could you re-trigger the CI build for the 2PRs?

Also, you could submit 1 or 2 more PRs with the next batch of changes.

cchung100m commented 5 years ago

@vandanavk,

Thank you for the suggestion.

I add some commits based on reviewers' suggestions and re-trigger the CI build. #13815 #13848

Consider there are parts of the changes remain on the 18 branches(each one has 15 files), should I open the other 18 PRs?

I would appreciate if you can give me further suggestions.

many thanks,

vandanavk commented 5 years ago

@cchung100m maintaining and addressing review comments for 18 PRs in parallel may be difficult. I suggest you keep 3-4 PRs open at a time.

looks like there is a CI failure on your PRs. the error doesn't give much information. maybe re-trigger it again to check.

vandanavk commented 5 years ago

@cchung100m will you be submitting the next batch of PRs for these fixes?

szha commented 4 years ago

examples for 2.0 onward will be maintained in https://github.com/apache/incubator-mxnet-examples