Skip to content
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

Revert "Updated _evaluate_pipelines to consolidate side effects" #1409

Merged
merged 2 commits into from Nov 5, 2020

Conversation

christopherbunn
Copy link
Contributor

Reverts #1337

@dsherry
Copy link
Collaborator

dsherry commented Nov 5, 2020

Yep, due to the test flake from #1408

Copy link
Collaborator

@dsherry dsherry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @christopherbunn ! 👍

@dsherry
Copy link
Collaborator

dsherry commented Nov 5, 2020

@christopherbunn for the release notes, I'd say you can just append "reverted :pr:`1409`" to the end. Then when we fix and re-merge, we can delete that entry and replace with the new one. That work?

@codecov
Copy link

codecov bot commented Nov 5, 2020

Codecov Report

Merging #1409 into main will increase coverage by 8.75%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1409      +/-   ##
==========================================
+ Coverage   91.21%   99.95%   +8.75%     
==========================================
  Files         213      213              
  Lines       13934    13938       +4     
==========================================
+ Hits        12708    13931    +1223     
+ Misses       1226        7    -1219     
Impacted Files Coverage Δ
evalml/automl/automl_search.py 99.62% <100.00%> (+0.58%) ⬆️
evalml/tests/automl_tests/test_automl.py 100.00% <100.00%> (ø)
evalml/utils/logger.py 100.00% <100.00%> (ø)
evalml/tests/component_tests/test_components.py 100.00% <0.00%> (+1.11%) ⬆️
...s/prediction_explanations_tests/test_algorithms.py 100.00% <0.00%> (+1.12%) ⬆️
evalml/utils/gen_utils.py 100.00% <0.00%> (+1.53%) ⬆️
evalml/tests/component_tests/test_utils.py 100.00% <0.00%> (+1.86%) ⬆️
evalml/tests/pipeline_tests/test_pipelines.py 100.00% <0.00%> (+3.39%) ⬆️
...derstanding/prediction_explanations/_algorithms.py 97.15% <0.00%> (+4.29%) ⬆️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9451546...cf8010c. Read the comment docs.

@christopherbunn
Copy link
Contributor Author

@christopherbunn for the release notes, I'd say you can just append "reverted :pr:1409" to the end. Then when we fix and re-merge, we can delete that entry and replace with the new one. That work?

@dsherry Since the changelog line for 1337 isn't there anymore with this revert PR, I think I'll just make a new entry for now. We can delete when we push it back in.

@@ -8,6 +8,7 @@ Release Notes
* Fixes
* Updated enum classes to show possible enum values as attributes :pr:`1391`
* Changes
* Reverted changes from :pr:`1337` :pr:`1409`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 cool, yep we can delete this when we merge a new PR

@christopherbunn christopherbunn merged commit 9d43af7 into main Nov 5, 2020
3 checks passed
@dsherry dsherry mentioned this pull request Nov 24, 2020
@freddyaboulton freddyaboulton deleted the revert-1337-1295_`_evaluate`_changes branch May 13, 2022 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants