Speed up unit tests - #2365
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2365 +/- ##
=======================================
+ Coverage 99.7% 99.7% +0.1%
=======================================
Files 281 281
Lines 24963 25014 +51
=======================================
+ Hits 24866 24917 +51
Misses 97 97
Continue to review full report at Codecov.
|
5879608 to
985f1ca
Compare
| git-test-dask: | ||
| pytest evalml/tests/automl_tests/dask_tests/ -n 1 --doctest-modules --cov=evalml/tests/automl_tests/dask_tests/ --junitxml=test-reports/junit.xml --doctest-continue-on-failure --timeout 300 | ||
|
|
||
| .PHONY: git-test-nocov |
There was a problem hiding this comment.
These commands are no longer used.
| "its core features. Please be mindful of that when running search()." | ||
| ) | ||
|
|
||
| self._SLEEP_TIME = 0.1 |
There was a problem hiding this comment.
One of the things I found is that the sleep time increases the run time for some unit tests that run many iterations.
This is a quick way to manipulate the amount of sleep time for some tests but it's not the only one! Let me know if you think there's a better way.
There was a problem hiding this comment.
Why is there a sleep time?
There was a problem hiding this comment.
I don't quite remember why this sleep time is placed here--could you explain? Will decreasing the sleep timer for those unit tests not affect functionality?
There was a problem hiding this comment.
This is the thread where we discussed the sleep in the dask pr.
We picked 0.1 just cause it seemed like a good number. Decreasing the sleep time will not change the functionality especially since these tests use the sequential engine.
| 0.1666874487986626: 1, | ||
| 0.13357573073236878: 1, | ||
| 0.06778096366056789: 1, | ||
| 0.05824028901694482: 63, |
There was a problem hiding this comment.
Since the data we use is different, the expected partial dependence is now different too
There was a problem hiding this comment.
Honestly, I'm still not sure I understand the usefulness of checking against part_dep_ans... how are we getting these numbers anyhow 🤔
|
|
||
| @patch("evalml.pipelines.BinaryClassificationPipeline.score") | ||
| @patch("evalml.pipelines.BinaryClassificationPipeline.fit") | ||
| @patch("evalml.tuners.skopt_tuner.Optimizer.tell") |
There was a problem hiding this comment.
Surprisingly, Optimizer.tell uses up a lot of the time for tests that run automl for many iterations.
| m_clf = ARIMARegressor(d=None) | ||
| m_clf.fit(X=X[:250], y=y[:250]) | ||
| y_pred = m_clf.predict(X=X[250:]) | ||
| m_clf.fit(X=X[:25], y=y[:25]) |
There was a problem hiding this comment.
I'm surprised this test took ~60 seconds on the original data size since its 500 rows.
I think we should file a separate issue to see where the bottleneck is.
There was a problem hiding this comment.
Also, do you think its worth just making the changes in the fixture itself?
There was a problem hiding this comment.
+1 to changing the fixture, or using the fixture to make a new smaller fixture for ARIMA specifically if we want to keep the original fixture
There was a problem hiding this comment.
Also maybe super optimization / nitpick but rather than having to update the number every time we want to change how much data we use, could be useful to set a variable... just in case we later decide we don't like the number 25 or something :3
There was a problem hiding this comment.
I changed the fixture to only have 50 rows but the test still manually splits the data into two halves for fitting and predicting. So that's why the 25 is there. Do you want me to define another fixture to represent the second half of the data?
There was a problem hiding this comment.
@freddyaboulton To address your original comment, ARIMA is especially slow when it has a hard time converging. It'll throw this warning Maximum Likelihood optimization failed to converge. and I think it would require different initialization parameters
| y = pd.Series(y) | ||
| X["dt_column"] = pd.Series(pd.date_range("20200101", periods=X.shape[0])) | ||
|
|
||
| random_dates = pd.Series(pd.date_range("20200101", periods=10)).sample( |
There was a problem hiding this comment.
In order to speed up these tests, I used fewer unique dates. I think this points to how our approach for handling dates does not scale super well. I think we should revisit this in the near future.
There was a problem hiding this comment.
Good to file an issue? :)
| ) | ||
| pipeline.fit(X, y) | ||
| for objective in binary_core_objectives: | ||
| for objective in ["Log Loss Binary", "AUC"]: |
There was a problem hiding this comment.
Running fewer objectives and columns through calculate_permutation_importance_one_column.
There was a problem hiding this comment.
Curious, would it be better to replace binary_core_objectives with something like a binary_test_objectives, then we could just reduce the load on all the tests at once. Additionally, is the fixture used after you pull it out? Seems kinda weird that lint didn't pick it up...
There was a problem hiding this comment.
Word I'd be down for changing the fixture! I just wanted to change the slowest tests first. What are good candidates for binary_test_objectives?
There was a problem hiding this comment.
I think F1, Log Loss, and AUC could be good choices
chukarsten
left a comment
chukarsten
left a comment
There was a problem hiding this comment.
I really like these changes and appreciate you taking the time to trudge through them. I think there are a few changes that can move into fixtures, which would remove a few hardcoded values from the code base. Also maybe start to pull more focus onto conf.py as a lever to turn up thoroughness of tests.
| "its core features. Please be mindful of that when running search()." | ||
| ) | ||
|
|
||
| self._SLEEP_TIME = 0.1 |
There was a problem hiding this comment.
Why is there a sleep time?
| max_iterations=3, | ||
| n_jobs=1, | ||
| ) | ||
| automl._SLEEP_TIME = 0.001 |
There was a problem hiding this comment.
Is there any reason not to just set the default sleep time as low as possible and not set it on a per-test level?
There was a problem hiding this comment.
My thought was just to modify the slowest tests. Most tests run few iterations so the sleep time doesn't contribute that much to the total time. I do think it'd be nice to leave the 0.1 value in AutoMLSearch but then define a fixture or something that sets it small for all tests. I'm not sure what the easiest way to do that is though and that would require modifying all evalml unit tests to add the fixture.
There was a problem hiding this comment.
Whatever you think is best! I think it's weird having the sleep time to begin with, so without any context on why we have it to begin with, my natural inclination is to just drop it for everyone in the lowest touch way possible. You ultimately make the decision on how you want to handle this, it's not blocking!
| clf.feature_importance == np.zeros(1) | ||
| with patch.object(clf, "_component_obj"): | ||
| clf.fit(X, y) | ||
| clf.feature_importance == np.zeros(1) |
There was a problem hiding this comment.
lol, is this intended to be an equality? or an assertion?
There was a problem hiding this comment.
Eagle eyes. Thank you hehehe
There was a problem hiding this comment.
lol you're just moving into the context manager....it was like that before you touched it
There was a problem hiding this comment.
😂 Anyways, should be an assert now.
| m_clf = ARIMARegressor(d=None) | ||
| m_clf.fit(X=X[:250], y=y[:250]) | ||
| y_pred = m_clf.predict(X=X[250:]) | ||
| m_clf.fit(X=X[:25], y=y[:25]) |
| m_clf = ARIMARegressor(d=None) | ||
| m_clf.fit(X=X[:250], y=y[:250]) | ||
| y_pred = m_clf.predict(X=X[250:]) | ||
| m_clf.fit(X=X[:25], y=y[:25]) |
There was a problem hiding this comment.
Also, do you think its worth just making the changes in the fixture itself?
| m_clf = ARIMARegressor(d=None) | ||
| m_clf.fit(X=None, y=y[:250]) | ||
| y_pred = m_clf.predict(X=None, y=y[250:]) | ||
| m_clf.fit(X=None, y=y[:25]) |
There was a problem hiding this comment.
Yea, I'm thinking just change the fixture to be 25 rows
| ) | ||
| pipeline.fit(X, y) | ||
| for objective in binary_core_objectives: | ||
| for objective in ["Log Loss Binary", "AUC"]: |
There was a problem hiding this comment.
Curious, would it be better to replace binary_core_objectives with something like a binary_test_objectives, then we could just reduce the load on all the tests at once. Additionally, is the fixture used after you pull it out? Seems kinda weird that lint didn't pick it up...
angela97lin
left a comment
angela97lin
left a comment
There was a problem hiding this comment.
This looks great, and is so, so thorough!! Mostly nit-picky comments, but I'd like to revisit our idea to use fail-fast as False. I think it could be better to keep that as true and move the dask tests outside the matrix, if possible, given the increased number of jobs and the number of concurrent jobs we can have. Curious about your thoughts on that, but otherwise, thank you for doing this!! The increased runtimes are super exciting. 😁
| @@ -1,87 +0,0 @@ | |||
| name: Linux Unit Tests (Dask Only) | |||
There was a problem hiding this comment.
To clarify: we're removing this because your matrix covers dask tests, correct?
| runs-on: ubuntu-latest | ||
| strategy: | ||
| fail-fast: true | ||
| fail-fast: false |
There was a problem hiding this comment.
Alternatively, I wonder if there's a way to keep fail-fast to true, and then move the dask tests outside of this matrix and have fail-fast to false for just the dask tests.
I think that'd be a good idea just because we're upping the number of jobs significantly now--right now, your PR has 43 jobs which is beyond the number of concurrent number of jobs allowed. If the tests failed expectedly (bug in PR, typo, etc.), we'd still have to wait for all of those tests to finish, potentially blocking the queue for other PRs.
There was a problem hiding this comment.
Do you just want to set fail-fast to true for all jobs? The thing I don't like about moving tests outside the test matrix is that it leads to a lot of duplicated code across the yamls because the install steps are the same. There may be a way to reuse code across actions but I'd rather just fail-fast to alleviate concerns over too many jobs running.
There was a problem hiding this comment.
I believe we're allowed a max of 180 total jobs and 50 macOS jobs under Enterprise so we should be fine I think?
| "its core features. Please be mindful of that when running search()." | ||
| ) | ||
|
|
||
| self._SLEEP_TIME = 0.1 |
There was a problem hiding this comment.
I don't quite remember why this sleep time is placed here--could you explain? Will decreasing the sleep timer for those unit tests not affect functionality?
| @pytest.mark.parametrize( | ||
| "automl_type", | ||
| [ProblemTypes.BINARY, ProblemTypes.MULTICLASS, ProblemTypes.REGRESSION], | ||
| [ProblemTypes.BINARY, ProblemTypes.MULTICLASS], |
| ) | ||
| @patch("evalml.automl.engine.sequential_engine.train_pipeline") | ||
| def test_automl_drop_index_columns(mock_train, mock_binary_score, X_y_binary): | ||
| @patch("evalml.pipelines.BinaryClassificationPipeline.fit") |
There was a problem hiding this comment.
wow curious, does this shave off time or is this just more correct?
There was a problem hiding this comment.
I don't think this was actually not mocking the pipeline fit before!
| m_clf = ARIMARegressor(d=None) | ||
| m_clf.fit(X=X[:250], y=y[:250]) | ||
| y_pred = m_clf.predict(X=X[250:]) | ||
| m_clf.fit(X=X[:25], y=y[:25]) |
There was a problem hiding this comment.
+1 to changing the fixture, or using the fixture to make a new smaller fixture for ARIMA specifically if we want to keep the original fixture
| m_clf = ARIMARegressor(d=None) | ||
| m_clf.fit(X=X[:250], y=y[:250]) | ||
| y_pred = m_clf.predict(X=X[250:]) | ||
| m_clf.fit(X=X[:25], y=y[:25]) |
There was a problem hiding this comment.
Also maybe super optimization / nitpick but rather than having to update the number every time we want to change how much data we use, could be useful to set a variable... just in case we later decide we don't like the number 25 or something :3
| 0.1666874487986626: 1, | ||
| 0.13357573073236878: 1, | ||
| 0.06778096366056789: 1, | ||
| 0.05824028901694482: 63, |
There was a problem hiding this comment.
Honestly, I'm still not sure I understand the usefulness of checking against part_dep_ans... how are we getting these numbers anyhow 🤔
| y = pd.Series(y) | ||
| X["dt_column"] = pd.Series(pd.date_range("20200101", periods=X.shape[0])) | ||
|
|
||
| random_dates = pd.Series(pd.date_range("20200101", periods=10)).sample( |
There was a problem hiding this comment.
Good to file an issue? :)
eb77b0d to
25d7e70
Compare
ParthivNaresh
left a comment
ParthivNaresh
left a comment
There was a problem hiding this comment.
Looking great! The execution time looks so much better now!
| runs-on: ubuntu-latest | ||
| strategy: | ||
| fail-fast: true | ||
| fail-fast: false |
There was a problem hiding this comment.
I believe we're allowed a max of 180 total jobs and 50 macOS jobs under Enterprise so we should be fine I think?
| - python_version: "3.7" | ||
| core_dependencies: false | ||
| codecov: false | ||
| dir: 'git-test-automl' |
There was a problem hiding this comment.
I really like the way you've split these up here
| include: | ||
| - python_version: "3.7" | ||
| core_dependencies: false | ||
| command: 'git-test-automl' |
There was a problem hiding this comment.
Just out of curiosity, why are these paths set to command vs dir from linux_unit_tests_with_latest_deps.yml ?
There was a problem hiding this comment.
No reason! dir is a leftover from a previous way I tried to parametrize the matrix. I changed them all to use command now.
| m_clf = ARIMARegressor(d=None) | ||
| m_clf.fit(X=X[:250], y=y[:250]) | ||
| y_pred = m_clf.predict(X=X[250:]) | ||
| m_clf.fit(X=X[:25], y=y[:25]) |
There was a problem hiding this comment.
@freddyaboulton To address your original comment, ARIMA is especially slow when it has a hard time converging. It'll throw this warning Maximum Likelihood optimization failed to converge. and I think it would require different initialization parameters
| ( | ||
| LinearPipelineWithDropCols, | ||
| {"Drop Columns Transformer": {"columns": ["country"]}}, | ||
| { |
| ) | ||
| pipeline.fit(X, y) | ||
| for objective in binary_core_objectives: | ||
| for objective in ["Log Loss Binary", "AUC"]: |
There was a problem hiding this comment.
I think F1, Log Loss, and AUC could be good choices
| name: Nightly Linux Unit Tests | ||
|
|
||
| on: | ||
| schedule: |
There was a problem hiding this comment.
These are what the nightlies look like. Before merge I'll remove the on: pull_request and update the cron to run at 3 am. I just added the on: pull_request to test it out. I'll also change the ref of the github action to say main.
So if this pr is merged, this is the expected behavior:
On pushes to prs and merges to main run:
- linux unit tests with latest versions python=3.8, core deps [True, False]. Measure coverage and upload to codecov.
- windows unit tests with python 3.8, core deps False. Don't measure coverage.
- linux unit tests with minimum versions with python 3.7, core deps False. Don't measure coverage.
At 3am
- linux unit tests with latest versions with python [3.7, 3.9], core deps False. Don't measure coverage.
- windows unit tests with python [3.7, 3.9], core deps False. Don't measure coverage.
- Not running minimum dependency versions as a nightly because some of the minimum package versions are not available for 3.9! I think just testing minimum versions with 3.7 is fine (and that's what we do now).
Sound good? The total number of unit test checks would be 17. FYI @chukarsten @dsherry
| name: Nightly ${{ matrix.python_version }} ${{matrix.command}} | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| fail-fast: false |
There was a problem hiding this comment.
Setting fail-fast to be false for the nightlies so that we know what all the failures are when they occur.
| ensembling=True, | ||
| ) | ||
| automl.search() | ||
| automl._SLEEP_TIME = 0.0001 |
There was a problem hiding this comment.
@freddyaboulton do you think there's a way to define a fixture for automl tests which handles stuff like _SLEEP_TIME, mocking pipeline methods and stuff like Optimizer.tell, etc?
What I'm thinking about is: what happens when we wanna add more automl tests? We can copy the patterns we'll add in this PR. But if there were a way to encode them in a fixture or some other easy-to-consume format, it would make it less likely that new tests inadvertently break the test patterns we want (speedups like mocking and sleep time).
There was a problem hiding this comment.
@dsherry Good point! Per our discussion at stand-up today, I'll merge this as-is but then look into a way we can package our best practices into a fixture!
There was a problem hiding this comment.
Sounds great! Sorry for any confusion, this comment is definitely not blocking this PR, and we can discuss after this PR is merged.
bchen1116
left a comment
bchen1116
left a comment
There was a problem hiding this comment.
Nice cleanup! Changes look good to me!
| ) | ||
| pipeline.fit(X, y) | ||
| for objective in multiclass_core_objectives: | ||
| for objective in ["Log Loss Multiclass", "AUC Micro"]: |
There was a problem hiding this comment.
Would be nice to do something similar to what @chukarsten pointed out here with multiclass_test_objectives
6 participants
Pull Request Description
Fixes #1815 by speeding up some unit tests and splitting up some of the jobs.
The changes follow the analysis in this document.
I think this is about a >=70% speedup in the runtime of the tests.
Splitting up the jobs is not necessary. I am including it in this PR mainly to show how it could be done. I prefer to just use a beefier machine if possible rather than adding more checks.
After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of
docs/source/release_notes.rstto include this pull request by adding :pr:123.