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 "Allow pipelines from AutoMLSearch to be pickled (#1721)" #1958

Merged
merged 9 commits into from Mar 10, 2021

Conversation

jeremyliweishih
Copy link
Contributor

Fixes #1912.

@codecov
Copy link

codecov bot commented Mar 10, 2021

Codecov Report

Merging #1958 (38f7ce0) into main (0b57961) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             main    #1958     +/-   ##
=========================================
- Coverage   100.0%   100.0%   -0.0%     
=========================================
  Files         269      268      -1     
  Lines       22188    22076    -112     
=========================================
- Hits        22182    22070    -112     
  Misses          6        6             
Impacted Files Coverage Δ
evalml/pipelines/__init__.py 100.0% <ø> (ø)
evalml/pipelines/utils.py 100.0% <ø> (ø)
.../automl_tests/test_automl_search_classification.py 100.0% <ø> (ø)
...ests/automl_tests/test_automl_search_regression.py 100.0% <ø> (ø)
evalml/automl/automl_search.py 100.0% <100.0%> (ø)
evalml/tests/automl_tests/test_automl.py 100.0% <100.0%> (ø)
evalml/tests/pipeline_tests/test_pipelines.py 100.0% <100.0%> (ø)

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 0b57961...38f7ce0. Read the comment docs.

@@ -47,7 +47,7 @@
"source": [
"__Note:__ To provide data to EvalML, it is recommended that you create a `DataTable` object using [the Woodwork project](https://woodwork.alteryx.com/en/stable/).\n",
"\n",
"EvalML also accepts ``pandas`` input, and will run type inference on top of the input ``pandas`` data. If you’d like to change the types inferred by EvalML, you can use the `infer_feature_types` utility method as follows. The `infer_feature_types` utility method takes pandas or numpy input and converts it to a Woodwork data structure. It takes in a `feature_types` parameter which can be used to specify what types specific columns should be. In the example below, we specify that the provider, which would have otherwise been inferred as a column with natural language, is a categorical column."
"EvalML also accepts ``pandas`` input, and will run type inference on top of the input ``pandas`` data. If you\u2019d like to change the types inferred by EvalML, you can use the `infer_feature_types` utility method as follows. The `infer_feature_types` utility method takes pandas or numpy input and converts it to a Woodwork data structure. It takes in a `feature_types` parameter which can be used to specify what types specific columns should be. In the example below, we specify that the provider, which would have otherwise been inferred as a column with natural language, is a categorical column."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I've been seeing it come up in other PRs as well. I don't know what's up but agree it doesn't look like a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same, looks fine though.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's so weird. This is the unicode character "right single quotation mark" 😆 perhaps jupyter changed the way they represent unicode somehow.

Copy link
Contributor

@angela97lin angela97lin left a comment

Choose a reason for hiding this comment

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

:shipit: ⛩️ !

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.

@jeremyliweishih Looks good to me! Thanks for doing this. I think adding some coverage is worth it but not blocking merge.

"outputs": [],
"source": [
"# saving the best pipeline using .save()\n",
"# best_pipeline.save(\"file_path_here\")\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

.save should still work right? Maybe we should add back this code snippet, but I agree we should delete pickle.dumps.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, save still works because it uses cloudpickle, not pickle

evalml/tests/automl_tests/test_automl.py Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

Looking good! I didn't have any comments but I agree with @freddyaboulton 's comments about the tests and the automl user guide. We should address the test comments at least before merge.

Copy link
Contributor

@bchen1116 bchen1116 left a comment

Choose a reason for hiding this comment

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

🚀

@jeremyliweishih jeremyliweishih merged commit 156f547 into main Mar 10, 2021
@dsherry dsherry mentioned this pull request Mar 11, 2021
@freddyaboulton freddyaboulton deleted the js_revert_1721 branch May 13, 2022 15:34
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.

AutoMLSearch get_pipeline always returns pipelines with the same name
5 participants