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

Address loading in pipeline that uses unloaded custom objective class code #113

Merged
merged 30 commits into from Oct 28, 2019

Conversation

angela97lin
Copy link
Contributor

@angela97lin angela97lin commented Oct 3, 2019

Addresses #83

Decided to explicitly tell and disallow users from loading in a pipeline that requires a custom objective unless the custom objective code is available (loaded).

@angela97lin angela97lin changed the title Catch exception when user wants to access unloaded class Catch exception when user wants to load in pipeline that uses unloaded custom objective class code Oct 3, 2019
evalml/pipelines/utils.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 8, 2019

Codecov Report

Merging #113 into master will increase coverage by 0.19%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #113      +/-   ##
==========================================
+ Coverage   94.32%   94.51%   +0.19%     
==========================================
  Files          58       58              
  Lines        1516     1514       -2     
==========================================
+ Hits         1430     1431       +1     
+ Misses         86       83       -3
Impacted Files Coverage Δ
evalml/__init__.py 100% <ø> (ø) ⬆️
evalml/tests/automl_tests/test_autoclassifier.py 100% <ø> (ø) ⬆️
evalml/tests/conftest.py 100% <ø> (ø) ⬆️
evalml/tests/pipeline_tests/test_pipelines.py 100% <100%> (+6.38%) ⬆️
evalml/pipelines/utils.py 100% <100%> (ø) ⬆️

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 9c995a2...d628f5c. Read the comment docs.

evalml/tests/pipeline_tests/test_pipelines.py Outdated Show resolved Hide resolved
@angela97lin angela97lin self-assigned this Oct 17, 2019
Copy link
Contributor

@rwedge rwedge left a comment

Did we switch design choices here on cloudpickle vs pickle?

evalml/tests/pipeline_tests/test_pipelines.py Outdated Show resolved Hide resolved
@angela97lin
Copy link
Contributor Author

angela97lin commented Oct 17, 2019

Yup, discussed with @kmax12 and decided that it should be reasonable for users to use a consistent version of Python and if users do run into the case where they're pickling/unpickling with different versions then we can address it then.

Copy link
Contributor

@rwedge rwedge left a comment

We should add cloudpickle to requirements.txt

@angela97lin angela97lin changed the title Catch exception when user wants to load in pipeline that uses unloaded custom objective class code test loading in pipeline that uses unloaded custom objective class code Oct 21, 2019
@angela97lin angela97lin changed the title test loading in pipeline that uses unloaded custom objective class code Address loading in pipeline that uses unloaded custom objective class code Oct 21, 2019
requirements.txt Show resolved Hide resolved
rwedge
rwedge previously approved these changes Oct 22, 2019
Copy link
Contributor

@rwedge rwedge left a comment

Looks good

jeremyliweishih
jeremyliweishih previously approved these changes Oct 23, 2019
kmax12
kmax12 previously approved these changes Oct 25, 2019
Copy link
Contributor

@kmax12 kmax12 left a comment

LGTM

@angela97lin angela97lin dismissed stale reviews from kmax12, jeremyliweishih, and rwedge via d628f5c Oct 26, 2019
@angela97lin angela97lin merged commit b91b3a7 into master Oct 28, 2019
@angela97lin angela97lin deleted the cloudpickle branch Oct 28, 2019
@angela97lin angela97lin mentioned this pull request Oct 29, 2019
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

4 participants