[MRG] circle-ci should only run/build plot_* files #12797

adrinjalali · 2018-12-16T21:22:57Z

Discovered in #12791 that if an example using the TkAgg matplotlib bcakend is touched, the doc build fails complaining that the tk library is not present.

This PR is to test this hypothesis, and then to propose a fix to those ubuntu images.

adrinjalali · 2018-12-16T22:26:35Z

It's not really that, I'm not sure if we should run this example at all. Ping @qinhanmin2014

jnothman · 2018-12-18T02:29:29Z

We should not ordinarily be running examples not preceded by plot_

adrinjalali · 2018-12-18T20:14:03Z

Although, I really think we should run the examples even if they're not touched, since many changes in the codebase do affect the examples.

adrinjalali · 2018-12-20T23:04:45Z

I guess this PR now "fixes" the issue which was introduced in #10407, but doesn't solve the larger issue of us missing the new problems introduced in examples due to some changes in the codebase. We have a few options to improve the situation:

Run all examples on all PRs and error on new warnings

cons: takes 4 times longer for each circle-ci job to finish (which I think still wouldn't be much more than appveyor or travis times)!
pros: detects all potential issues in examples immediately, and the same PR would have to deal with it

Keep the script as is for PRs, run all examples always on master and fail on warnings. This should be feasible once we merge [MRG] DOC Fix warnings in examples #12654 and fix the remaining issues.

cons: pretty frequently master will go red and mostly core-devs would have to deal with it.
pros: issues are detected immediately, and ci jobs in PRs won't take long.

Keep the script as is, for master and the PRs, but run the docs in a daily cron job and error on warnings

cons: issues are detected by some of the core devs and they'll have to deal with it.
pros: master stays green, and PR CIs run fast

Leave things as is, but we kinda try to remember to run the examples ourselves rather frequently locally and report or fix the issues.

cons: this is mostly what we have now, and it doesn't seem like warnings are kept at a low count.
pros: status quo on the CIs are kept, things run fast.

If all options are actually feasible, I personally prefer option 1; but I guess there are legitimate reasons for not running the examples all the time, and I have no idea about the capacity of circle-ci and if it can handle the load if we switch to 1.

ogrisel · 2018-12-21T09:32:56Z

Thanks for the summary @adrinjalali.

Maybe we can try to go with 1 and see if the impact on the CI duration is unbearable.

rth

Yeah, we can try running all plot_ examples if the runtime is manageable.

qinhanmin2014

Do you know how we did this before #10407 @adrinjalali I'm pretty sure we have a way to do this previously.

qinhanmin2014 · 2018-12-22T15:41:38Z

build_tools/circle/build_doc.sh

@@ -51,7 +51,7 @@ get_build_type() {
    fi
    git_range="origin/master...$CIRCLE_SHA1"
    git fetch origin master >&2 || (echo QUICK BUILD: failed to get changed filenames for $git_range; return)
-    filenames=$(git diff --name-only $git_range)
+    filenames=$(git diff --name-only $git_range -- `find . -name plot_*`)


According to the logs, maybe we should filter these files at changed_examples below?

yeah, didn't think it's important, but now I applied the same filter there.

This seems inconsistent with our output

filenames=XXX if [ -z "$filenames" ] echo QUICK BUILD: no changed filenames for $git_range return changed_examples=XXX if [[ -n "$changed_examples" ]] echo BUILD: detected examples/ filename modified in $git_range: $changed_examples return echo QUICK BUILD: no examples/ filename modified in $git_range:

so seems that filenames include all the files, not only examples.

adrinjalali · 2018-12-22T23:17:06Z

Do you know how we did this before #10407 @adrinjalali I'm pretty sure we have a way to do this previously.

I haven't checked yet. In the meantime, I'll let this PR be as is. We can either have it as a temporary solution, or wait until the other solution is ready.

qinhanmin2014 · 2018-12-23T01:45:34Z

I haven't checked yet. In the meantime, I'll let this PR be as is. We can either have it as a temporary solution, or wait until the other solution is ready.

I don't think I'll approve if we're unable to figure out how we did this previously (i.e., how #10407 introduce the regression). Unfortunately I'm unable to figure this out.

qinhanmin2014 · 2018-12-23T01:47:43Z

ping @lesteve what's your opinion on this? How did we filter examples which do not begin with plot_ previously? Thanks.

adrinjalali · 2018-12-27T08:49:10Z

@qinhanmin2014 I'm not sure what it is that you're not approving though.

qinhanmin2014 · 2018-12-27T15:18:53Z

(1) I think filenames should include all the files, not only examples. See https://github.com/scikit-learn/scikit-learn/pull/12797/files#r243750313
(2) I'm not familiar with our CI scripts so I'd like to know how did the regression happen (i.e., how did we filter examples which do not begin with plot_ previously). I think we need these insights to avoid introducing bugs.
I don't mind if someone else give +2 and merge this one.

qinhanmin2014 · 2018-12-27T15:41:20Z

And I've found #8849: Run more examples that do not start with plot_ on CircleCI :)

adrinjalali · 2018-12-27T17:30:59Z

@qinhanmin2014 there are two situations where we build the docs. In some cases we build all examples (building the master or release branches for instance), and when we choose to run only the changed examples (on PRs for instance).

Before #10407 we were running all examples on all PRs, and #10407 was a proposal to only run changed files, and was accepted. The issue is that sphinx-gallery by default only chooses the files with a plot_ prefix in their name (unless something else specified in doc/conf.py) , and #10407 changed that behavior to only changed files. The issue was that "changed files" sometimes include files that don't start with plot_, and this PR fixes this issue.

Regarding the issue of including all example and documentation files, I changed the filter pattern to include all .rst files as well. But I don't think it's needed, really, not sure.

Now if at some point #8849 gets in, and we haven't switched to what we're proposing here (i.e. going back to always running all examples), it should also modify the filter introduced in this PR, to include also files starting with run_. But that PR itself is more about when we run all examples, and not when we select only the changed ones.

The issue you pointed out about affected_doc_paths only affects what we show in _changed.html, which is not crucial and we already do recognize that that file is not accurate. Run filtering the files for that file only introduces some extra false positives there.

This all assumes we keep the proposal of #10407, and the discussion here is about reverting that, and always returning BUILD in get_build_type no matter which files are touched (unless some other directive such as [doc skip] is present in the commit message).

I hope this clears up things a bit.

amueller · 2018-12-27T21:52:07Z

Took me a while to get what's happening but #10407 overwrote sphinx_gallery_conf.filename_pattern which removed the plot_ filtering. It's a bit ugly that we reimplement that filtering here (in a slightly different way), but I think the false positives are probably negligible?

so +1 for this PR

build_tools/circle/build_doc.sh

qinhanmin2014 · 2018-12-28T11:39:33Z

build_tools/circle/build_doc.sh

@@ -57,7 +57,7 @@ get_build_type() {
        echo QUICK BUILD: no changed filenames for $git_range
        return
    fi
-    changed_examples=$(echo "$filenames" | grep -e ^examples/)
+    changed_examples=$(echo "$filenames" | grep -E "^examples/(.*/)*plot_")


grep -e seems enough?

@adrinjalali Hmm, I tried locally and seems that grep -E won't work, I'm not familiar with -E.

the -E just gives special meaning to ( and ), works for me. For me it's equivalent to grep -e "^examples/$.*/$*plot_"

Apologies I went through your code too quickly.

qinhanmin2014

LGTM if CIs are green

lesteve · 2019-01-02T10:23:01Z

A bit late to the party, but I agree with @amueller #12797 (comment). It is a bit unfortunate to have the plot_ regex in two places but seems the most reasonable thing to do.

A few comments:

running all the examples on each PR touching the examples folder was painful if memory serves me right. When someone works on an example you just want to quickly double-check the output on this example and not have to wait on all the examples to run.
because we run all the examples on master we catch problems very quickly after the PR is merged anyway. I think those are quite rare cases and I don't remember something like this happening since [MRG+1] CircleCI: run only modified examples in CircleCI #10407 was merged although I may have missed it if happened more recently.
you can always add a commit with [doc build] in the message to force the full rebuild if you want to double-check that all your examples run before merging.

…n#12797)" This reverts commit 705eb4e.

adrinjalali mentioned this pull request Dec 18, 2018

MRG Drop legacy python / remove six dependencies #12639

Merged

adrinjalali changed the title ~~[WIP] Fix the tk bacend issue in circle-ci~~ [MRG] circle-ci should only run/build plot_* files Dec 18, 2018

adrinjalali added 5 commits December 18, 2018 20:53

touch an example with tk requirement

419912d

add tcl and tk to circle-ci

e92a300

revert tcl tk installation

2ffd588

limit to doc build to plot_* files

eec5a8f

remove the import __future__ line

c970755

adrinjalali force-pushed the ci/tk branch from f139dd3 to c970755 Compare December 18, 2018 19:53

rth approved these changes Dec 22, 2018

View reviewed changes

qinhanmin2014 reviewed Dec 22, 2018

View reviewed changes

also apply the filter when detecting changed files

e4ade5f

adrinjalali added 2 commits December 27, 2018 17:52

revert changed files filter

4bf448d

include all rst files as well

ff6f363

amueller approved these changes Dec 27, 2018

View reviewed changes

qinhanmin2014 reviewed Dec 28, 2018

View reviewed changes

build_tools/circle/build_doc.sh Outdated Show resolved Hide resolved

move the filter to change_examples

bd5b4de

adrinjalali mentioned this pull request Dec 28, 2018

MNT CI run all examples if some code is changed #12880

Closed

qinhanmin2014 reviewed Dec 28, 2018

View reviewed changes

build_tools/circle/build_doc.sh Outdated Show resolved Hide resolved

fix the grep line

69f9109

qinhanmin2014 reviewed Dec 28, 2018

View reviewed changes

qinhanmin2014 approved these changes Dec 28, 2018

View reviewed changes

qinhanmin2014 merged commit 77b9b1f into scikit-learn:master Dec 28, 2018

adrinjalali added a commit to adrinjalali/scikit-learn that referenced this pull request Jan 7, 2019

BLD circle-ci should only run/build plot_* files (scikit-learn#12797)

5b74898

qinhanmin2014 mentioned this pull request Feb 19, 2019

Release 0.20.3 #13186

Merged

17 tasks

jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Feb 20, 2019

BLD circle-ci should only run/build plot_* files (scikit-learn#12797)

17a6c9d

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

BLD circle-ci should only run/build plot_* files (scikit-learn#12797)

705eb4e

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Revert "BLD circle-ci should only run/build plot_* files (scikit-lear…

f75167d

…n#12797)" This reverts commit 705eb4e.

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Revert "BLD circle-ci should only run/build plot_* files (scikit-lear…

c523cdd

…n#12797)" This reverts commit 705eb4e.

koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019

BLD circle-ci should only run/build plot_* files (scikit-learn#12797)

446da03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] circle-ci should only run/build plot_* files #12797

[MRG] circle-ci should only run/build plot_* files #12797

adrinjalali commented Dec 16, 2018

adrinjalali commented Dec 16, 2018

jnothman commented Dec 18, 2018 via email

adrinjalali commented Dec 18, 2018

adrinjalali commented Dec 20, 2018

ogrisel commented Dec 21, 2018

rth left a comment

qinhanmin2014 left a comment

qinhanmin2014 Dec 22, 2018

adrinjalali Dec 22, 2018

qinhanmin2014 Dec 23, 2018

adrinjalali commented Dec 22, 2018

qinhanmin2014 commented Dec 23, 2018 •

edited

Loading

qinhanmin2014 commented Dec 23, 2018

adrinjalali commented Dec 27, 2018

qinhanmin2014 commented Dec 27, 2018

qinhanmin2014 commented Dec 27, 2018

adrinjalali commented Dec 27, 2018

amueller commented Dec 27, 2018

qinhanmin2014 Dec 28, 2018

qinhanmin2014 Dec 28, 2018

adrinjalali Dec 28, 2018

qinhanmin2014 Dec 28, 2018

qinhanmin2014 left a comment

lesteve commented Jan 2, 2019

[MRG] circle-ci should only run/build plot_* files #12797

[MRG] circle-ci should only run/build plot_* files #12797

Conversation

adrinjalali commented Dec 16, 2018

adrinjalali commented Dec 16, 2018

jnothman commented Dec 18, 2018 via email

adrinjalali commented Dec 18, 2018

adrinjalali commented Dec 20, 2018

ogrisel commented Dec 21, 2018

rth left a comment

Choose a reason for hiding this comment

qinhanmin2014 left a comment

Choose a reason for hiding this comment

qinhanmin2014 Dec 22, 2018

Choose a reason for hiding this comment

adrinjalali Dec 22, 2018

Choose a reason for hiding this comment

qinhanmin2014 Dec 23, 2018

Choose a reason for hiding this comment

adrinjalali commented Dec 22, 2018

qinhanmin2014 commented Dec 23, 2018 • edited Loading

qinhanmin2014 commented Dec 23, 2018

adrinjalali commented Dec 27, 2018

qinhanmin2014 commented Dec 27, 2018

qinhanmin2014 commented Dec 27, 2018

adrinjalali commented Dec 27, 2018

amueller commented Dec 27, 2018

qinhanmin2014 Dec 28, 2018

Choose a reason for hiding this comment

qinhanmin2014 Dec 28, 2018

Choose a reason for hiding this comment

adrinjalali Dec 28, 2018

Choose a reason for hiding this comment

qinhanmin2014 Dec 28, 2018

Choose a reason for hiding this comment

qinhanmin2014 left a comment

Choose a reason for hiding this comment

lesteve commented Jan 2, 2019

qinhanmin2014 commented Dec 23, 2018 •

edited

Loading