[MRG+2] Update repo to work with both new and old scikit-learn

[wdevazelhes]

Fixes #311

• I added the workaround suggested by @bellet here: https://github.com/scikit-learn-contrib/metric-learn/issues/311#issuecomment-804229956, so that imports work both in sklearn <0.24 and >= 0.24 (EDIT: actually the old import strategy was already deprecated in 0.22, so I compared the version to 0.22)
• as well as an additional travis job to test (I chose the one with python 3.6 since it's the oldest python so I thought if someting goes wrong it might be this one, and also with skggm, since I also I'm thinking something might have more chance to go wrong when using another package... I could have put all the checks (python3.6+3.7, with/without skggm), but then I thought the travis test suite might take quite some time)

[wdevazelhes]

Actually, tests are failing because of some LMNN problem similar to the discussion in #309 : a test is run on LMNN with classes that have too few labels. The fact that this pb happens in only some travis tests could be due to some different random states depending on the order/quantity of runned tests depending on the particular travis test (with/without skggm) (+ I guess the change of library versions wrt to the past could maybe impact the seed? otherwise I don't know why tests were passing before) so I set the random seed in the failing test so that they should all either fail or pass After having a consistent behaviour we can decide what to do (either adapt the test to have big enough classes, or adapt LMNN to accept classes with few labels) I'll react on #309 accordingly since the decision should also impact the new LMNN PR (for instance we can decide to merge it as is since actually the current LMNN is even more strict than the newer PR (it needs at least n_neighbors + 1 elements in each class, not just 2), so the new PR would actually allow to deal with more cases, and we could deal with singleton classes in another PR)

[wdevazelhes]

Ok so there's just the travis test with the old scikit learn that is failing, because of the string representation of estimators, I'll look into that But at least fixing the random seed indeed fixed the other tests (temporarily since we should not depend on the random seed)

[wdevazelhes]

I guess the PR is ready to merge :)

[wdevazelhes]

Thanks for the review @terrytangyuan! I adressed your comment

[wdevazelhes]

Thanks for the review @perimosocordiae ! I'll do the changes and set the PR to [MRG] as soon as I finish

[wdevazelhes]

@perimosocordiae I adressed your comments, I guess the PR is now ready to merge :)

[bellet]

Somehow we do not see all checks (it happens sometimes), maybe do an empty commit to make sure @wdevazelhes?

[wdevazelhes]

Thanks for the review @bellet !

I tried to do an empty commit but we still see 1 line for the check, indeed it's weird... Although clicking on details we see all checks, so maybe there was a change of display of travis checks on github ?

[wdevazelhes]

@perimosocordiae, do you approve my updates after your review ?

[bellet]

Thanks for the review @bellet !

I tried to do an empty commit but we still see 1 line for the check, indeed it's weird... Although clicking on details we see all checks, so maybe there was a change of display of travis checks on github ?

We don't see test coverage though for some reason

[wdevazelhes]

We don't see test coverage though for some reason

Really ? If I click on "show all checks" and then "details", here is what I see:

[perimosocordiae]

Merged, thanks!

[bellet]

We don't see test coverage though for some reason

Really ? If I click on "show all checks" and then "details", here is what I see:

Where do you see test coverage (codecov)?

[wdevazelhes]

Where do you see test coverage (codecov)?

Ah yes that's right sorry ! Actually they sent me an email telling me that their bash uploader was hacked: https://about.codecov.io/security-update/?utm_medium=email&utm_source=marketo&ajs_uid=2386266&ajs_event=Clicked%20Email&mkt_tok=MzMyLUxWWC03NDEAAAF8dJ5mOA5H1E5ORbGsBiNKeiALhIlihm9VPW70i0qLTSuToRz_FC1YHHXfXcvCiatj0yHQxAfkDKCri3j9ksaM0_8pH36YziLCqWSOcdIt So I guess the pb is probably because of that, I'll try to look at it see if they say any guideline for fixing the pb

[wdevazelhes]

Merged, thanks!

Thanks @perimosocordiae ! I'll try to look into the codecov pb noticed by @bellet (see above), to double check that this PR indeed checks the coverage requirement

[wdevazelhes]

It seems the codecov is OK here is what I see on codecov in the "pull" section:

[bellet]

Where do you see test coverage (codecov)?

Ah yes that's right sorry ! Actually they sent me an email telling me that their bash uploader was hacked: https://about.codecov.io/security-update/?utm_medium=email&utm_source=marketo&ajs_uid=2386266&ajs_event=Clicked%20Email&mkt_tok=MzMyLUxWWC03NDEAAAF8dJ5mOA5H1E5ORbGsBiNKeiALhIlihm9VPW70i0qLTSuToRz_FC1YHHXfXcvCiatj0yHQxAfkDKCri3j9ksaM0_8pH36YziLCqWSOcdIt So I guess the pb is probably because of that, I'll try to look at it see if they say any guideline for fixing the pb

Good catch! They have guidelines on what to do on the above page

[wdevazelhes]

I opened a draft PR here #314, running their recommended env command, and it doesn't seem to leak any sensitive information (what env prints can be seen for instance ), (I was wondering about the SSH_CONNECTION variable that is printed but I checked and it contains no password just IP adresses and port numbers)

As a precaution, on app.codecov.io, I clicked on "Regenerate" for the "Repository Upload Token", but I don't even think it had any effect, since I searched for "codecov" in our code, and I see nowhere any token

So I think we are safe,

However codecov still doesn't appear in the "all checks" of the test PR #314, so I think we can wait a little bit: they may be still fixing problems In the meantime we can still check manually the code coverage for next PRs directly on the codecov website.