Skip to content

Look at lgtm.com alerts for last couple of months #12167

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

Closed
jnothman opened this issue Sep 26, 2018 · 17 comments · Fixed by #24592
Closed

Look at lgtm.com alerts for last couple of months #12167

jnothman opened this issue Sep 26, 2018 · 17 comments · Fixed by #24592
Labels
Easy Well-defined and straightforward way to resolve good first issue Easy with clear instructions to resolve help wanted

Comments

@jnothman
Copy link
Member

Due to a technical glitch, lgtm.com alerts have not been reported on our pull requests for the last couple of months. They could have caught bugs like #12154.

While most of the alerts added in the last couple of months are to sklearn/externals which we can largely ignore (or we can tell joblib and liac-arff that they have issues), it would be good to look through other alerts in the repo to find any that aren't false alarms.

A first step for a contributor here would be to sort through https://lgtm.com/projects/g/scikit-learn/scikit-learn and identify alerts that are candidates to be fixed. (I have spotted at least one unused import alert that we can fix.)

@jnothman jnothman added Easy Well-defined and straightforward way to resolve good first issue Easy with clear instructions to resolve help wanted labels Sep 26, 2018
@rth
Copy link
Member

rth commented Sep 26, 2018

loky related issues can be reported under https://github.com/tomMoral/loky/issues/172 while for joblib there is an existing issue in joblib/joblib#535

@s0
Copy link

s0 commented Sep 26, 2018

Hey, I see that PR integration seems to now have been resolved in #12166, sorry to hear that we'd silently been not analysing PRs.

I have a few suggestions that could help the current situation, and help mitigate similar problems going forwards:

  • You could add LGTM as a required check before merging, this would have caught the issue of LGTM not getting the webhooks, but will also catch most other misconfigurations that would prevent LGTM from analysing a PR or posting a status, as well as any technical issues our side.
  • You could add LGTM badges to the README, there is one badge that displays the alert count, that you would have seen going up (even if PR integration was disabled or not working): https://lgtm.com/projects/g/scikit-learn/scikit-learn/ci/

On our side, we're discussing implementing additional checks to detect when situations like this occur, and notify the user.

Beyond this, you may be interested in a competition that we're running for fixing alerts (now that you have a few new ones): https://competitions.lgtm.com/ghu-2018 We'll donate to the WWF for every alert fixed until the end of October, and there are also multiple rounds with different prizes, the first one ending at the end of this month, with a grand prize of a free ticket to GitHub Universe (along with travel and accommodation).

@thomasjpfan
Copy link
Member

LGTM has been red on master for a while: https://lgtm.com/projects/g/scikit-learn/scikit-learn/overview/ They all have the same error regarding C/C++.

@s0
Copy link

s0 commented Jan 25, 2019

@thomasjpfan Thanks for pointing that out. We've taken a look and have got the LGTM analysis working again now.

@s0
Copy link

s0 commented Jan 25, 2019

Further info on fixes here: #13044

@amueller
Copy link
Member

Current issues seem relevant again

@s0
Copy link

s0 commented Jul 12, 2019

@amueller could you elaborate? Builds and analysis seem to be working as far as i can see?

@amueller
Copy link
Member

What I meant is "everything is working fine, you can look at the alerts and fix them" which I think was the original issue.

@s0
Copy link

s0 commented Jul 12, 2019

Ah gotcha 👍

@amueller
Copy link
Member

amueller commented Jul 11, 2020

Lol our current "_more_tags" solution is tagged as an issue by lgtm. Maybe we should silence it? I'm not sure how else to deal with it tbh.

@kurchi1205
Copy link

kurchi1205 commented Aug 20, 2021

Hi , I want to make my first contribution , Can I work on the slice issue in lgtm.com

@dhivyasreedhar
Copy link

Hi, can I work on this? I'm new so can someone guide me?

@pree-T
Copy link

pree-T commented Dec 29, 2021

Hello !May I work on this? Can someone guide me?

@Jared-Glenn
Copy link

Hi there, I'm fairly new at this, but would love to contribute. Can someone help me get started here?

@jjerphan
Copy link
Member

jjerphan commented Aug 4, 2022

@jnothman: should we reintroduce LGTM in PR checks? I have never used this tool, at first sight it looks informative but I do not know if it created frictions in the team past usage.

@thomasjpfan
Copy link
Member

LGTM.com checks are already in the PR checks: https://github.com/scikit-learn/scikit-learn/pull/24051/checks under: "LGTM.com".

The LGTM.com check breaks CI from time to time because of C++ analysis and the way it needs the base branch to build correctly. For example, #23493 fixes C++ analysis, but LGTM.com fails because the main branch does not have the fix.

Given that we do not have any C++ or javascript alerts, I tried to turn C++ and javascript off a while ago: #14025 but there was no way to do it. Currently, I still do not see a way to turn it off for those languages.

TLDR: LGTM.com is already running on PRs and I think it's still somewhat useful.

@haiatn
Copy link
Contributor

haiatn commented Oct 4, 2022

Hi,
for new ones wanting to help there are currently 8 errors to fix here:
https://lgtm.com/projects/g/scikit-learn/scikit-learn/alerts/?mode=list&severity=error

Note that LGTM is shutting down on December 16th 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Easy Well-defined and straightforward way to resolve good first issue Easy with clear instructions to resolve help wanted
Projects
None yet