Skip to content

Have all_components and all_pipelines try to initialize components/pipelines #849

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

Merged
merged 2 commits into from
Jun 12, 2020

Conversation

dsherry
Copy link
Contributor

@dsherry dsherry commented Jun 12, 2020

Fix #523

Update all_components() and all_pipelines() to try to initialize components and pipelines, and if there are failures related to import, skip them.

This replaces the hacky import_or_raise stuff I had hard-coded for our optional modeling deps xgboost and catboost.

@codecov
Copy link

codecov bot commented Jun 12, 2020

Codecov Report

Merging #849 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #849   +/-   ##
=======================================
  Coverage   99.68%   99.68%           
=======================================
  Files         195      195           
  Lines        7738     7740    +2     
=======================================
+ Hits         7714     7716    +2     
  Misses         24       24           
Impacted Files Coverage Δ
evalml/exceptions/__init__.py 100.00% <100.00%> (ø)
evalml/exceptions/exceptions.py 100.00% <100.00%> (ø)
evalml/pipelines/components/utils.py 100.00% <100.00%> (ø)
evalml/pipelines/pipeline_base.py 100.00% <100.00%> (ø)
evalml/pipelines/utils.py 100.00% <100.00%> (ø)
evalml/tests/component_tests/test_utils.py 97.05% <100.00%> (+0.08%) ⬆️

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 bcc9242...25368c2. Read the comment docs.

@dsherry dsherry marked this pull request as ready for review June 12, 2020 02:17
Base automatically changed from ds_522_standardize_components to master June 12, 2020 14:34
Copy link
Collaborator

@jeremyliweishih jeremyliweishih left a comment

Choose a reason for hiding this comment

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

This is really nice to have - I like the standardization of returning instances (like how handle_component works)!

@@ -92,6 +92,6 @@ def handle_component(component):
raise ValueError("handle_component only takes in str or ComponentBase")
components = all_components()
if component not in components:
raise KeyError("Component {} was not found".format(component))
raise MissingComponentError('Component "{}" was not found'.format(component))
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!

component = handle_component(component)
try:
component = handle_component(component)
except MissingComponentError as e:
Copy link
Collaborator

Choose a reason for hiding this comment

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

very nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks yeah I like the way this fits together now!

@dsherry dsherry force-pushed the ds_523_init_components_pipelines branch from 7745a61 to 25368c2 Compare June 12, 2020 14:57
@dsherry dsherry merged commit 92db722 into master Jun 12, 2020
@dsherry dsherry deleted the ds_523_init_components_pipelines branch June 12, 2020 15:15
@angela97lin angela97lin mentioned this pull request Jun 30, 2020
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.

Have all_components and all_pipelines try to initialize components/pipelines
2 participants