Add test to ensure binary pipeline maintains threshold after pickling #3027
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3027 +/- ##
=======================================
+ Coverage 99.8% 99.8% +0.1%
=======================================
Files 312 312
Lines 30162 30183 +21
=======================================
+ Hits 30073 30094 +21
Misses 89 89
Continue to review full report at Codecov.
|
| ) | ||
|
|
||
|
|
||
| def test_pickled_pipeline_preserves_threshold( |
There was a problem hiding this comment.
Can we add some of this logic to test_serialization_protocol or test_load_pickled_pipeline_with_custom_objective instead?
There was a problem hiding this comment.
@freddyaboulton both of those tests seem to use cloudpickle, but I wanted to use pickle. Would it be better to edit those tests to include my pickle test, or leave the test as is?
There was a problem hiding this comment.
Got it, I see! I wonder if it's possible to merge the tests. We could have a different test fixture than pickled_pipeline_path that uses pickle instead of cloudpickle and move some of the logic from this test in to test_load_pickled_pipeline_with_custom_objective.
Up to you though. If it's better to keep the tests separate, I'm down for it but maybe let's move this test next to test_load_pickled_pipeline_with_custom_objective so that the pickling tests are at least grouped.
freddyaboulton
left a comment
freddyaboulton
left a comment
There was a problem hiding this comment.
Looks good to me @bchen1116 ! Left a non-blocking suggestion for how to organize these tests.
| ) | ||
|
|
||
|
|
||
| def test_pickled_pipeline_preserves_threshold( |
There was a problem hiding this comment.
Got it, I see! I wonder if it's possible to merge the tests. We could have a different test fixture than pickled_pipeline_path that uses pickle instead of cloudpickle and move some of the logic from this test in to test_load_pickled_pipeline_with_custom_objective.
Up to you though. If it's better to keep the tests separate, I'm down for it but maybe let's move this test next to test_load_pickled_pipeline_with_custom_objective so that the pickling tests are at least grouped.
…bc_pickle_threshold
2 participants
Testing thresholds