-
-
Notifications
You must be signed in to change notification settings - Fork 26k
MNT add durations=20 to makefile to show test runtimes locally #11147
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
Conversation
should this go in setup.cfg?
|
should showlocals also go there? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In its current state this LGTM.
I don't think that putting --duration
into setup.cfg
is a good idea though, as when one selects a single test with pytest it will show its setup/teardown/run time which we don't care about (just tried it).
+1 for putting --showlocals
there though.
Though one can always just select and rerun the failed test locally with In some way using pytest defaults, unless there is a reason not to, might be easier. |
I'm not sure about showlocals... I think it might give pretty long tracebacks sometimes. |
Why do you think providing the runtime of a single test is a bad idea? I think it's good because it reminds us to be careful. |
Yeah, before this PR,
with
IMO this is just visual noise - can tell just by running it that it takes 2 s (or that some test is slow as you did in #11146). |
can we set it to a smaller number in setup.cfg, and to 20 in Travis?
|
With respect to putting it in Pytest already prints cumulated run time by default so when selecting a couple of tests this doesn't bring anything relevant. For all test suite, why not, but we already have this in Travis. Run time shouldn't differ wildly between local runs and Travis unless there is an issue with the implementation (as might be the case with #11146 (comment) ). Also if this is added, and a contributor doesn't want it, he/she would need to keep a local copy of |
I would be in favour of leaving things as they are too. Locally I run selected tests a lot more than the full test suite, so adding a
That would be my suggestion too. |
@lesteve but ok with adding to makefile? |
@rth was the mean shift already the slowest by far when you debugged it? If you didn't notice it then, maybe something in joblib broke in the meantime? |
As you saw in #11146 (comment) it has only been around 3 weeks since that regression appeared. TBH, the slowdown in that tests was noticeable when running tests locally lately but I never took the time to investigate. As to the rest, in the slowest tests from your #11146 (comment) |
Sorry accidental key press :) |
Happy to go back to my original change of adding only to the makefile, which runs all the tests. |
I am fine with adding |
I agree that printing durations would not likely have fixed the current
issue. A CI that told us if there was an unexpected increase in test time
might be a better alert, but false positives would be rife due to
environmental factors
|
9fbd1aa
to
f46b745
Compare
reverted to just doing it in the makefile |
Merging with Loic's +1 in #11147 (comment). Thanks! |
* tag '0.20rc1': (1109 commits) MNT rc version DOC Release dates for 0.20 (scikit-learn#11838) DOC Fix: require n_splits > 1 in TimeSeriesSplit (scikit-learn#11937) FIX xfail for MacOS LogisticRegressionCV stability (scikit-learn#11936) MNT: Use GEMV in enet_coordinate_descent (Pt. 1) (scikit-learn#11896) [MRG] TST/FIX stop optics reachability failure on 32bit (scikit-learn#11916) ENH add multi_class='auto' for LogisticRegression, default from 0.22; default solver will be 'lbfgs' (scikit-learn#11905) MAINT Fix test_logistic::test_dtype_match failure on 32 bit arch (scikit-learn#11899) DOC Updated link to Laurens van der Maaten's home page (scikit-learn#11907) DOC Remove stray backtick in /doc/modules/feature_extraction.rst (scikit-learn#11910) Deprecate min_samples_leaf and min_weight_fraction_leaf (scikit-learn#11870) MNT modify test_sparse_oneclasssvm to be parametrized (scikit-learn#11894) EXA set figure size to avoid overlaps (scikit-learn#11889) MRG/REL fixes /skips for 32bit tests (scikit-learn#11879) add durations=20 to makefile to show test runtimes locally (scikit-learn#11147) DOC loss='l2' is no longer accpeted in l1_min_c DOC add note about brute force nearest neighbors for string data (scikit-learn#11884) DOC Change sign of energy in RBM (scikit-learn#11156) RFC try to warn on iid less often (scikit-learn#11613) DOC reduce plot_gpr_prior_posterior.py warnings(scikit-learn#11664) ...
* releases: (1109 commits) MNT rc version DOC Release dates for 0.20 (scikit-learn#11838) DOC Fix: require n_splits > 1 in TimeSeriesSplit (scikit-learn#11937) FIX xfail for MacOS LogisticRegressionCV stability (scikit-learn#11936) MNT: Use GEMV in enet_coordinate_descent (Pt. 1) (scikit-learn#11896) [MRG] TST/FIX stop optics reachability failure on 32bit (scikit-learn#11916) ENH add multi_class='auto' for LogisticRegression, default from 0.22; default solver will be 'lbfgs' (scikit-learn#11905) MAINT Fix test_logistic::test_dtype_match failure on 32 bit arch (scikit-learn#11899) DOC Updated link to Laurens van der Maaten's home page (scikit-learn#11907) DOC Remove stray backtick in /doc/modules/feature_extraction.rst (scikit-learn#11910) Deprecate min_samples_leaf and min_weight_fraction_leaf (scikit-learn#11870) MNT modify test_sparse_oneclasssvm to be parametrized (scikit-learn#11894) EXA set figure size to avoid overlaps (scikit-learn#11889) MRG/REL fixes /skips for 32bit tests (scikit-learn#11879) add durations=20 to makefile to show test runtimes locally (scikit-learn#11147) DOC loss='l2' is no longer accpeted in l1_min_c DOC add note about brute force nearest neighbors for string data (scikit-learn#11884) DOC Change sign of energy in RBM (scikit-learn#11156) RFC try to warn on iid less often (scikit-learn#11613) DOC reduce plot_gpr_prior_posterior.py warnings(scikit-learn#11664) ...
* dfsg: (1109 commits) MNT rc version DOC Release dates for 0.20 (scikit-learn#11838) DOC Fix: require n_splits > 1 in TimeSeriesSplit (scikit-learn#11937) FIX xfail for MacOS LogisticRegressionCV stability (scikit-learn#11936) MNT: Use GEMV in enet_coordinate_descent (Pt. 1) (scikit-learn#11896) [MRG] TST/FIX stop optics reachability failure on 32bit (scikit-learn#11916) ENH add multi_class='auto' for LogisticRegression, default from 0.22; default solver will be 'lbfgs' (scikit-learn#11905) MAINT Fix test_logistic::test_dtype_match failure on 32 bit arch (scikit-learn#11899) DOC Updated link to Laurens van der Maaten's home page (scikit-learn#11907) DOC Remove stray backtick in /doc/modules/feature_extraction.rst (scikit-learn#11910) Deprecate min_samples_leaf and min_weight_fraction_leaf (scikit-learn#11870) MNT modify test_sparse_oneclasssvm to be parametrized (scikit-learn#11894) EXA set figure size to avoid overlaps (scikit-learn#11889) MRG/REL fixes /skips for 32bit tests (scikit-learn#11879) add durations=20 to makefile to show test runtimes locally (scikit-learn#11147) DOC loss='l2' is no longer accpeted in l1_min_c DOC add note about brute force nearest neighbors for string data (scikit-learn#11884) DOC Change sign of energy in RBM (scikit-learn#11156) RFC try to warn on iid less often (scikit-learn#11613) DOC reduce plot_gpr_prior_posterior.py warnings(scikit-learn#11664) ...
This is already happening on travis. Times on travis and locally are quite different sometimes, though.