singa-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Moaz Reyad <m...@apache.org>
Subject Re: [DISCUSS] Improve code quality
Date Thu, 05 Mar 2020 10:26:49 GMT
Dear Chris,

OK. lgtm is enabled now in SINGA.

Yes, the checks can be only for reference if the reported errors are not
very critical. But if a big problem is found by lgtm, then may be the pull
request should be revised and fixed before the merge.

Also the lgtm checks can be configured with .lgtm.yml file. (See:
https://help.semmle.com/lgtm-enterprise/user/help/lgtm.yml-configuration-file.html
)

Thanks,
Moaz

On Thu, Mar 5, 2020 at 2:54 AM Yeung Sai Ho <dcsysh@nus.edu.sg> wrote:

> Dear Moaz,
>
> Thank you so much for your email.
>
> It is great to have LGTM to check the PR. It think it is a bit similar to
> cpplint adapted recently for PR checking.
>
> Meanwhile, since previously the existing PRs had some issues on cpplint.
> So I am quite worrying about existing PRs, whether they can pass the test.
>
> One important thing to note is that I am worrying about the future hotfix
> will not be able to be merged to the master branch if it is checked by
> LGTM. It is because there are some LGTM errors/warning in master branch
> currently.
>
> So if it is possible, I suggest we can do in two stages:
>
> Stage 1. Let LGTM check the PR first. However, if the PR cannot pass the
> LGTM test, we can still merge the PR. The LGTM check is for reference only.
>
> Stage 2. In the trial of PR checking by LGTM for some months, we can
> modify our code to fulfills the LGTM rules. Then we can start using LGTM to
> decide whether to merge the PR.
>
> Thank you so much!
>
> Best Regards,
> Chris
>
>
>
>
>
> > Moaz Reyad <moaz@apache.org> 於 2020年3月4日 18:13 寫道:
> >
> >         - External Email -
> >
> >
> >
> > Dear all,
> >
> > Here are two updates about lgtm:
> >
> > 1. Just few days after our discussion, GitHub announced that they
> acquired Semmle (the company that made lgtm). So lgtm is now part of
> GitHub. (See https://github.blog/2019-09-18-github-welcomes-semmle/)
> >
> > 2. Apache INFRA now has lgtm installed in the Apache GitHub organization
> account. We can enable the lgtm pull request integration on SINGA repo if
> everyone agrees.
> >
> > Thanks
> > Moaz
> >
> >> On 2019/09/16 04:20:43, Moaz Reyad <moaz@apache.org> wrote:
> >> Thank you all for participating in the discussion. It is nice that
> everyone
> >> agrees that code quality is important.
> >>
> >> Here are some comments:
> >>
> >> 1. It seems that everyone agrees on cpplint and pylint, but no one
> >> discussed lgtm. Do you agree on using also the lgtm tool which is
> proposed
> >> in [1] ? or do you think it is not needed and the cpplint and pylint is
> >> enough? Note that there are lgtm badges that can be added to SINGA
> readme
> >> on Github and acts as a certificate of the code quality that is visible
> to
> >> everyone. While cpplint and pylint are not as visible as lgtm. Also lgtm
> >> seems to report other problems that cpplint and pylint do not recognize.
> >> Please share your ideas about the lgtm tool and SINGA-484 [1] issue.
> >>
> >> 2. We need to separate the discussion of the problem of future code
> quality
> >> and the problem of current code quality. Enforcing code quality in
> future
> >> is great, but in my opinion the team must take some actions also for the
> >> current code. There are hundreds of issues reported by the three tools
> as
> >> mentioned in my previous email. Not all of them are important of course,
> >> but some percentage of them need to be fixed.
> >>
> >> 3. For future code quality, it is a good idea to use continuous
> integration
> >> to run static code checking (cpplint, pylint and also RAT). I can open a
> >> ticket in jira for this and anyone is welcome to implement it (after
> >> finishing the more urgent issues with Travis CI). But cpplint and pylint
> >> test will show too many errors already on the current code in the master
> >> branch. This is why I think we need to solve the problem of current code
> >> quality before enforcing future code quality.
> >>
> >> 4. The official site of SINGA is [2]. When the user navigates to "How to
> >> Contribute Code" link, he will go to [3]. The link that was mention in
> [4]
> >> is not accessible from [2]. The English site was moved to the root as
> the
> >> default language [5], so no need to use "/en" folder in the path. If you
> >> use the latest site build script [6], it will not create "/en" folder.
> This
> >> means the link [4] was not generated by the latest site build script
> [6].
> >>
> >> 5. The code style instructions in [4] can be useful only for code style
> >> issues, but all other code problems (such as those hundreds of issues
> >> reported by the three tools) are not only about code style. Check the
> LGTM
> >> alerts for SINGA [7] for example, they are not code style or readability
> >> issues. (e.g unused variables, unreachable code, ..)
> >>
> >> best regards,
> >> Moaz
> >>
> >> [1] https://issues.apache.org/jira/projects/SINGA/issues/SINGA-484
> >> [2] http://singa.apache.org
> >> [3] http://singa.apache.org/develop/contribute-code.html
> >> [4] http://singa.apache.org/en/develop/contribute-code.html
> >> [5] https://issues.apache.org/jira/projects/SINGA/issues/SINGA-436
> >> [6] https://github.com/apache/incubator-singa/blob/master/doc/build.sh
> >> [7]
> https://lgtm.com/projects/g/apache/incubator-singa/alerts/?mode=list
> >>
> >>> On Sun, Sep 15, 2019 at 5:02 PM Yeung Sai Ho <dcsysh@nus.edu.sg>
> wrote:
> >>>
> >>> P.S. Therefore, tools like cpplint and pylint are useful and can be
> >>> integrated into CI process that can check code automatically.
> >>>
> >>> 從我的 iPhone 傳送
> >>>
> >>> Yeung Sai Ho <dcsysh@nus.edu.sg<mailto:dcsysh@nus.edu.sg>> 於
> 2019年9月15日
> >>> 下午10:18 寫道:
> >>>
> >>>
> >>> Dear Prof. Moaz Reyad,
> >>>
> >>>
> >>>
> >>> Nice to discuss with you, I am currently with NUS as a research fellow,
> >>> and working with my peer co-workers (e.g. shicong/dcslin, joddly,
> shicheng,
> >>> wanqi, pinpom, etc.) on SINGA leaded by our boss. I am Chris Yeung and
> this
> >>> is my first time to join the discussion here.
> >>>
> >>>
> >>>
> >>> I agree with you. As a researcher and developer, I totally feel the
> same
> >>> since code quality is very important. The code analysis tool could
> save our
> >>> time and improve coding quality.
> >>>
> >>>
> >>>
> >>> In this email, please let me share my own personal view, concerning
> some
> >>> recent daily practice in our local team which is currently using to
> enhance
> >>> code quality:
> >>>
> >>>
> >>>
> >>> We are currently using the tools like cpplint and pylint to check and
> >>> reformat the code so that we make sure our code is in compliance with
> the
> >>> google style.
> >>>
> >>>
> >>>
> >>> Concerning our SINGA website, the new developers are suggested to read
> the
> >>> following SINGA doc web page before they join developing the SINGA,
> which
> >>> gives a complete and useful guide for their development:
> >>>
> >>> https://singa.apache.org/en/develop/contribute-code.html
> >>>
> >>>
> >>>
> >>> The above website recommends a very simple approach to enforce the
> Google
> >>> coding styles: We can use the VS Code editor (
> >>> https://code.visualstudio.com) and set the linting and formating
> tools.
> >>> First, we need to install the C/C++ extension and python extension.
> Then,
> >>> we can edit the seetings.json as:
> >>>
> >>> "editor.formatOnSave": true,
> >>>
> >>> "python.formatting.provider": "yapf",
> >>>
> >>> "python.formatting.yapfArgs": [
> >>>
> >>>    "--style",
> >>>
> >>>    "{based_on_style: google}"
> >>>
> >>> ],
> >>>
> >>> "python.linting.enabled": true,
> >>>
> >>> "python.linting.lintOnSave": true,
> >>>
> >>> "C_Cpp.clang_format_style": "Google"
> >>>
> >>>
> >>>
> >>> A reference settings.json file can be found here:
> >>>
> >>> https://gist.github.com/nudles/3d23cfb6ffb30ca7636c45fe60278c55
> >>>
> >>>
> >>>
> >>> The developers are suggested to fix the format errors before submitting
> >>> the PRs (pull requests).
> >>>
> >>>
> >>>
> >>> In addition to the code quality improving tool, we also have the tools
> for
> >>> adding documentations. The procedures to contribute for the
> documentation
> >>> is in our SINGA doc web page:
> >>>
> >>> https://singa.apache.org/en/develop/contribute-docs.html
> >>>
> >>>
> >>>
> >>> In our recent development, we always perform peer discussion in the
> local
> >>> team, formally meeting with our boss almost everyday, and seek advise
> from
> >>> our boss whenever any problems arise. We learn from each other and
> improve
> >>> ourselves gradually.
> >>>
> >>>
> >>>
> >>> The above is just to share my own personal view, concerning our recent
> >>> experience that also concerns code quality. Thank you very much for
> >>> listening!
> >>>
> >>>
> >>>
> >>> Best Regards,
> >>>
> >>> Research Fellow
> >>>
> >>> Chris YEUNG Sai Ho (
> >>> https://scholar.google.com.sg/citations?user=9XAJsd0AAAAJ&hl=en and
> >>> https://github.com/chrishkchris)
> >>>
> >>>
> >>>
> >>> Sent from Mail<https://go.microsoft.com/fwlink/?LinkId=550986> for
> >>> Windows 10
> >>>
> >>>
> >>>
> >>> From: Wang Wei<mailto:wangwei@comp.nus.edu.sg>
> >>> Sent: Sunday, 15 September 2019 2:38 PM
> >>> To: dev@singa.incubator.apache.org<mailto:
> dev@singa.incubator.apache.org>
> >>> Cc: dev@singa.apache.org<mailto:dev@singa.apache.org>
> >>> Subject: Re: [DISCUSS] Improve code quality
> >>>
> >>>
> >>>
> >>> Hi Moaz,
> >>>
> >>> I agree with you.
> >>> We did take some efforts to improve the code quality.
> >>> [1] introduces some tools for enforcing the coding style.
> >>> [2] introduces some tools for adding documentations.
> >>>
> >>> The current issue is that our contributors may have applied different
> >>> coding styles and tools using their different editors.
> >>> I suggest to do some tests during the CI process, e.g., running the
> cpplint
> >>> and pylint.
> >>> If all tests pass, then we merge the PR.
> >>>
> >>> Thanks!
> >>>
> >>> Best,
> >>> Wei
> >>>
> >>> [1]. http://singa.apache.org/en/develop/contribute-code.html
> >>> [2]. http://singa.apache.org/en/develop/contribute-docs.html
> >>>
> >>> On Sun, Sep 15, 2019 at 2:36 AM Moaz Reyad <moaz@apache.org<mailto:
> >>> moaz@apache.org>> wrote:
> >>>
> >>>> Dear team,
> >>>>
> >>>> Since SINGA is going to graduate soon from the incubator, I propose
to
> >>> use
> >>>> some tools to ensure high code quality. These tools check for known
> >>>> problems in the code and provide a detailed report for fixing them.
> May
> >>> be
> >>>> some code came from scientific experimental projects. We need to
> improve
> >>>> this code according to industry standards, so it can be used with more
> >>> real
> >>>> life projects.
> >>>>
> >>>> 1. I propose to add the code quality tools (cpplint[1], pylint[2] and
> >>>> lgtm[3]) to SINGA contribution guideline[4], so that each developer
is
> >>>> encouraged to install and run code quality checks in his local repo
> and
> >>> fix
> >>>> any problems before creating a pull request.
> >>>>
> >>>> 1.A CPP Lint: running cpplint in the src directory shows 822 errors,
> >>> while
> >>>> running in the include directory shows 708 errors. The guidelines [4]
> has
> >>>> an outdated information that instructs developers to use an old
> >>>> non-existing file tool/cpplint.py.
> >>>>
> >>>> 1.B Python Lint: running pylint in python/singa shows 5.00/10 rating,
> >>> while
> >>>> running in python/rafiki shows 0.00/10 rating.
> >>>>
> >>>> 1.C LGTM :There is a Jira ticket for adding LGTM badges to the
> README[5],
> >>>> so the quality of the code becomes more clear to everyone. LGTM pull
> >>>> request automation can't be enabled in Apache repo due to infra
> >>>> restrictions[6], but it works on personal forks of the project.
> Currently
> >>>> LGTM rates both C++ and Python code in SINGA as grade D.
> >>>>
> >>>> 2. I propose also to give the code quality higher priority in the next
> >>>> release since it is probably going to be the first release after
> >>>> graduation. The team is invited to fix as much as possible from the
> >>> current
> >>>> code issues and to use tools that check their new code before pushing
> it
> >>> to
> >>>> SINGA. Let's try to make the lgtm grade and lint rating as high as
> >>>> possible.
> >>>>
> >>>> Improving code quality is required to attract new users and
> developers.
> >>>> Users will trust more the project with better code and developers
> will be
> >>>> happy to contribute to it. It will also make the code review process
> >>> easier
> >>>> and more productive instead of wasting time in finding and fixing
> known
> >>>> code problems.
> >>>>
> >>>> New developers (or old developers who did not contribute for a while
> and
> >>>> would like to warm up) can start working on fixing lgtm and lint
> issues,
> >>>> since they are usually easy and there is a clear explanation of the
> >>> problem
> >>>> and how to solve it.
> >>>>
> >>>> What do you think?
> >>>>
> >>>> p.s. This discussion is the first topic in a series of proposals to
> >>> improve
> >>>> SINGA as it will be an Apache top level project soon. The next
> proposal
> >>>> will discuss improving the build and test pipeline in a separate
> thread
> >>> to
> >>>> avoid discussing too many things in one thread.
> >>>>
> >>>> best regards,
> >>>> Moaz
> >>>>
> >>>> [1] https://pypi.org/project/cpplint/
> >>>> [2] https://www.pylint.org/
> >>>> [3] https://lgtm.com/
> >>>> [4] http://singa.apache.org/develop/contribute-code.html
> >>>> [5] https://issues.apache.org/jira/projects/SINGA/issues/SINGA-484
> >>>> [6] https://issues.apache.org/jira/browse/INFRA-17954
> >>>>
> >>>
> >>> ________________________________
> >>>
> >>> Important: This email is confidential and may be privileged. If you are
> >>> not the intended recipient, please delete it and notify us
> immediately; you
> >>> should not copy or use it for any purpose, nor disclose its contents
> to any
> >>> other person. Thank you.
> >>>
> >>
>
> ________________________________
>
> Important: This email is confidential and may be privileged. If you are
> not the intended recipient, please delete it and notify us immediately; you
> should not copy or use it for any purpose, nor disclose its contents to any
> other person. Thank you.
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message