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

Move component importing to runtime and include test to check for warnings #1045

Merged
merged 9 commits into from Aug 13, 2020

Conversation

jeremyliweishih
Copy link
Contributor

@jeremyliweishih jeremyliweishih commented Aug 12, 2020

Fixes #1017. Docs here.

@jeremyliweishih jeremyliweishih self-assigned this Aug 12, 2020
def test_default_parameters(cls):

assert cls.default_parameters == cls().parameters, f"{cls.__name__}'s default parameters don't match __init__."


@pytest.mark.parametrize("cls", all_components())
def test_default_parameters_raise_no_warnings(cls):
with warnings.catch_warnings(record=True) as w:
Copy link
Contributor Author

@jeremyliweishih jeremyliweishih Aug 12, 2020

Choose a reason for hiding this comment

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

This test catches warnings with record=True and asserts none have been raised.

Copy link
Collaborator

@dsherry dsherry Aug 13, 2020

Choose a reason for hiding this comment

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

Awesome

Copy link
Collaborator

@dsherry dsherry Aug 13, 2020

Choose a reason for hiding this comment

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

So simple!

@codecov
Copy link

codecov bot commented Aug 12, 2020

Codecov Report

Merging #1045 into main will increase coverage by 5.59%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1045      +/-   ##
==========================================
+ Coverage   94.31%   99.91%   +5.59%     
==========================================
  Files         183      183              
  Lines       10156    10167      +11     
==========================================
+ Hits         9579    10158     +579     
+ Misses        577        9     -568     
Impacted Files Coverage Δ
evalml/pipelines/components/utils.py 100.00% <100.00%> (ø)
evalml/tests/component_tests/test_components.py 99.60% <100.00%> (+0.20%) ⬆️
evalml/tests/component_tests/test_estimators.py 100.00% <100.00%> (ø)
evalml/tests/component_tests/test_utils.py 100.00% <100.00%> (+3.57%) ⬆️
evalml/tests/conftest.py 100.00% <100.00%> (ø)
...peline_tests/explanations_tests/test_algorithms.py 100.00% <100.00%> (+1.11%) ⬆️
evalml/tests/pipeline_tests/test_pipelines.py 100.00% <100.00%> (+1.48%) ⬆️
evalml/automl/automl_search.py 99.55% <0.00%> (+0.44%) ⬆️
... 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 12be7f4...9f46eaf. Read the comment docs.

_all_transformers = get_importable_subclasses(Transformer, used_in_automl=False)
all_components = _all_estimators + _all_transformers

def _all_estimators():
Copy link
Contributor

@freddyaboulton freddyaboulton Aug 12, 2020

Choose a reason for hiding this comment

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

@jeremyliweishih If the solution is to make sure that components don't issue warnings when they are initialized with default arguments, do we need to change these to be functions instead of just lists?

Copy link
Contributor Author

@jeremyliweishih jeremyliweishih Aug 12, 2020

Choose a reason for hiding this comment

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

@freddyaboulton The idea with changing these lists to methods is so that even if warnings or errors occur they do so on runtime instead of import-time which I think is a better design choice as it is less confusing for users (plus the warnings or errors only generate at where they're called in the docs instead of whenever evalml is imported).

Copy link
Contributor

@freddyaboulton freddyaboulton Aug 12, 2020

Choose a reason for hiding this comment

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

I see but my hypothesis is that if we make sure components don't issue warnings on init we wouldn't see warnings regardless of whether _all_estimators is a function or a list?

Copy link
Contributor Author

@jeremyliweishih jeremyliweishih Aug 12, 2020

Choose a reason for hiding this comment

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

@freddyaboulton got it and I definitely agree with it! If we do make sure warnings do not surface (which should happen with the test) it might be better just to keep the lists the same to reduce change. What do you think @dsherry?

Copy link
Collaborator

@dsherry dsherry Aug 13, 2020

Choose a reason for hiding this comment

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

Great discussion!

@freddyaboulton I think its dangerous for us to continue instantiating components at import-time. If we ever add a component in the future which prints out a warning message by default, or has a stack trace, we can't even import the library! This way, import always works, even if an individual component doesn't. It's a future-proofing safety change.

We can have a separate discussion about how to validate component definitions. In fact that's tracked by #513

Copy link
Contributor

@freddyaboulton freddyaboulton Aug 13, 2020

Choose a reason for hiding this comment

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

@dsherry I see your point!

Copy link
Contributor

@angela97lin angela97lin left a comment

The current implementation LGTM :o

Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

Looks good! Thanks for making the change @jeremyliweishih !

Copy link
Collaborator

@dsherry dsherry left a comment

Awesome! Nice test :)

def test_default_parameters(cls):

assert cls.default_parameters == cls().parameters, f"{cls.__name__}'s default parameters don't match __init__."


@pytest.mark.parametrize("cls", all_components())
def test_default_parameters_raise_no_warnings(cls):
with warnings.catch_warnings(record=True) as w:
Copy link
Collaborator

@dsherry dsherry Aug 13, 2020

Choose a reason for hiding this comment

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

Awesome

def test_default_parameters(cls):

assert cls.default_parameters == cls().parameters, f"{cls.__name__}'s default parameters don't match __init__."


@pytest.mark.parametrize("cls", all_components())
def test_default_parameters_raise_no_warnings(cls):
with warnings.catch_warnings(record=True) as w:
Copy link
Collaborator

@dsherry dsherry Aug 13, 2020

Choose a reason for hiding this comment

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

So simple!

@jeremyliweishih jeremyliweishih merged commit 977eacf into main Aug 13, 2020
@dsherry dsherry mentioned this pull request Aug 25, 2020
@freddyaboulton freddyaboulton deleted the js_1017_text_warning branch May 13, 2022
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.

Text featurizer warning on evalml import
4 participants