Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LGTM: semantic checker issues found in vw python & html/js code #1442

Closed
arielf opened this issue Apr 4, 2018 · 19 comments

Comments

Projects
None yet
5 participants
@arielf
Copy link
Collaborator

commented Apr 4, 2018

lgtm found several semantic issues with some of our python code in the tree
(unfortunately, they don't support C/C++ yet, but their python/js catches seem to be pretty good):

https://lgtm.com/projects/g/JohnLangford/vowpal_wabbit/alerts/?mode=list

Files with issues:

  • utl/vw-hyperopt.py
  • demo/recall_tree/wikipara/WikiExtractor.py
  • python/vowpalwabbit/pyvw.py
  • python/vowpalwabbit/sklearn_vw.py
  • utl/active_interactor.py
  • utl/vw-lda
  • utl/vw-validate.html

Issues include Loop variable capture, call to non-callable, nested loops using same variable, use before set and more...

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Apr 7, 2018

I see C++ issues deeper in the warnings. @Scott-Graham-Bose do you want to look through the python ones? I'll handle the C++ ones.

@Scott-Graham-Bose

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2018

i can take a look

gramhagen pushed a commit to gramhagen/vowpal_wabbit that referenced this issue Apr 9, 2018

[WIP] addressing python semantic checking warnings from issue VowpalW…
…abbit#1442, needs a bit more testing to make sure changes did not break anything
@sjvs

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2018

Hi @Scott-Graham-Bose, @JohnLangford, @arielf. Glad to see that you're enjoying LGTM! I just came across this issue and wanted to mention that we do now support C/C++ analysis. Your project's C++ code has in fact already been analysed by LGTM; here's an overview of just those alerts: https://lgtm.com/projects/g/JohnLangford/vowpal_wabbit/alerts/?mode=list&lang=cpp

I see you've already set up some CI checks for pull requests; it's easy to add automatic code review by LGTM as well! Repository admins can do that from this page: https://lgtm.com/projects/g/JohnLangford/vowpal_wabbit/ci/

Here's an example of how LGTM is helping the AMPHTML project with automatic code review: ampproject/amphtml#13060

@arielf

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 27, 2018

Looks like quite a few (84) good C++ semantic issues that will be good to fix.

@Scott-Graham-Bose

This comment has been minimized.

Copy link
Contributor

commented May 3, 2018

#1476 PR to address these

@sjvs

This comment has been minimized.

Copy link
Contributor

commented May 3, 2018

LGTM 🙂

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

I'm running into an issue trying to use lgtm. In essence, it seems like the analysis is quite out of date. If I go to this: https://lgtm.com/projects/g/JohnLangford/vowpal_wabbit/alerts/?mode=list&lang=cpp URL, I only see the analysis from 5 days ago (a June 27 checkin). Why doesn't it autorefresh against the most current? And how can it be made to update faster?

@sjvs

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2018

Hi @JohnLangford! As is shown at the top of that page you linked, those are indeed the active alerts as of d177f77. Looking at the GitHub commit history, that was the most recent commit until a9bba04 was committed about 15 hours ago (I can't see when that was pushed to GitHub exactly). Since then, you added another two commits.

At the moment, LGTM continuously analyses just under 80k repositories. To avoid overloading GitHub, we poll every one of these repositories for new changes once roughly every 24 hours (on a randomised schedule). The last time we polled the vowpal_wabbit repo was yesterday evening at 17:16:11 UTC. That's why your more recent commits haven't been analysed yet.

There's an easy way to get (much!) quicker feedback on your changes: enable automatic code review for pull requests. Here are two examples of how that works for two other projects: AMPHTML and Systemd. If you enable PR integration, LGTM will analyse the commits as soon as a PR is opened/updated. This means you get results quicker, and more importantly: before any problems are merged into master 🙂. You can enable automatic code review for PRs here: https://lgtm.com/projects/g/JohnLangford/vowpal_wabbit/ci/

I've also just trigger another poll manually now to get you the latest results on master as well. Let me know if there's anything else I can help with 🙂

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

Thanks. I'll look into setting up the continuous integration in an hour or two.

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

FYI, I've added LGTM to the CI checks.
@deaktator @jmorra you may be heartened to know that the java code passes with flying colors :-)

I've reduced the set of C++ issues to a handful that I'll whittle down further over time. @Scott-Graham-Bose it seems like there are still some python issues beyond your existing patch. Perhaps you can look into them if you can spare the time.

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

(Some of the python is probably stuff I need to look into as well.)

@sjvs

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2018

Hi @JohnLangford! I just noticed you also have a good number of lines of C# code. We're currently testing out our C# analysis technology on LGTM; I hope you don't mind that I've added you to the closed beta. Results are now available here: https://lgtm.com/projects/g/JohnLangford/vowpal_wabbit/alerts/?mode=tree&lang=csharp.

How's the automatic code review for pull requests working for you? Thoughts/comments/suggestions are always welcome!

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Jul 4, 2018

@Scott-Graham-Bose

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2018

I've created a PR which updates some of code to resolve the warnings in the main python code, however there are more problems with python code in the demo and utl directories.

Two of the files generating a large portion of warnings are:

  1. demo/recall_tree/wikipara/WikiExtractor.py
  2. utl/active_interactor.py

WikiExtractor seems to be an external piece of code not maintained here, so my preference would be to remove it and provide a link to where to find it in the readme for that folder.

active_interactor is a bit difficult for me to parse, and I'm not sure it is in a currently working state. If this is functionality we want to maintain it would be worth overhauling it to make sure it's doing what we want and cleaning up the code.

In general I'm wondering if we should exclude the demo directory from these types of checks. I could see arguments either way.

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Jul 5, 2018

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Jul 16, 2018

@sjvs , it seems lgtm is suffering spurious fails. See #1533 Is this a known issue?

@JohnLangford JohnLangford reopened this Jul 16, 2018

@sjvs

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

Hi @JohnLangford: my best guess is that there was a network outage at our cloud provider. Both C# and Java analysis timed out on fetching external dependencies (from nuget.org and repo.maven.apache.org, respectively). I've now retriggered the analysis for that PR and will keep an eye on it. If it happens again then we should obviously have a closer look... Thanks for letting me know!

@sjvs

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

It turns out that there are wider connectivity issues: https://discuss.lgtm.com/t/lgtm-has-been-failing-to-get-packages-from-archive-ubuntu-com-since-yesterday/1112

We're investigating and will report back in the forum topic.

@jackgerrits

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

Closing this as LGTM is working normally now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.