Upgrade to support Featuretools 1.0.0 and nlp-primitives 2.0.0 by chukarsten · Pull Request #2848 · alteryx/evalml

chukarsten · 2021-09-27T16:33:26Z

Addresses: #2816

codecov · 2021-09-29T17:58:57Z

Codecov Report

Merging #2848 (988eaaf) into main (3948c5d) will increase coverage by 0.1%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main   #2848     +/-   ##
=======================================
+ Coverage   99.7%   99.7%   +0.1%     
=======================================
  Files        302     302             
  Lines      28394   28396      +2     
=======================================
+ Hits       28301   28303      +2     
  Misses        93      93

Impacted Files	Coverage Δ
...ponents/transformers/preprocessing/featuretools.py	`100.0% <100.0%> (ø)`
...ents/transformers/preprocessing/text_featurizer.py	`100.0% <100.0%> (ø)`
...rs/preprocessing/transform_primitive_components.py	`100.0% <100.0%> (ø)`
evalml/tests/component_tests/test_featuretools.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 3948c5d...988eaaf. Read the comment docs.

chukarsten · 2021-09-29T16:41:48Z

core-requirements.txt

@@ -12,8 +12,8 @@ psutil>=5.6.6
 requirements-parser>=0.2.0
 shap>=0.36.0
 texttable>=1.6.2


When featuretools and nlp-primitives release, remove the "rc1"s. Featuretools requires the update to ww 0.8.1. Bumped the minimum of the reqs to match the new minima for dask and pandas.

Should we do the ww 0.8.1 upgrade in a separate PR which we merge first? If this PR is ready to go I suppose it doesn't really matter at this point 😆

chukarsten · 2021-09-29T16:43:47Z

evalml/pipelines/components/transformers/preprocessing/featuretools.py

    def _make_entity_set(self, X):
        """Helper method that creates and returns the entity set given the input data."""
        ft_es = EntitySet()
+        del(X.ww)


I need feedback here. The reason I did this was to try to accommodate X being a WW initialized but unnamed dataframe. If I delete the accessor, the add_dataframe() function will follow its branch to re-initialize the dataframe and name it with the dataframe_name param. If X remains unnamed but WW initialized, the add_dataframe raises a series of Exceptions, the first being that it requires a named dataframe.

I think the del is fine here as long as we document the reasoning for it (and we can't find a better solution). Alternatively, we could name and then remove the name as well which makes for a more involved solution but doesn't include a scary del here!

Should we file a featuretools issue to allow for dataframes with ww and without names?

Rather than deleting the accessor, can we just give the dataframe a name?

I agree with @freddyaboulton here, it'd be nice if featuretools could support ww dataframes without names. I'm okay with not blocking this though, and filing an issue to track replacing this once we make the feature request and it gets merged :)

I think the issue here is potentially a bit bigger than just the name. Currently if a ww dataframe is passed to EntitySet.add_dataframe featuretools is expecting that everything is already set up and ready to go, meaning any values passed in for index, time_index or make_index (and any other arguments) will be ignored.

That means even if you add a name to the dataframe before passing it to add_dataframe, Featuretools will still not work as you expect because the index and make_index values will be ignored.

If you also need to set the index or use make index during the add call, we are going to have to change the add_dataframe code in Featuretools to use those values.

Given that, I think the best bet for now is to stick with the del(X.ww) approach...

chukarsten · 2021-09-29T17:18:51Z

evalml/tests/component_tests/test_featuretools.py

@@ -50,19 +50,19 @@ def test_featuretools_index(mock_calculate_feature_matrix, mock_dfs, X_y_multi):
    feature = DFSTransformer()
    feature.fit(X_new_index)
    feature.transform(X_new_index)


I'm not 100% sure on this change. @thehomebrewnerd do you have any feedback on this one? I wasn't sure whether entities was supposed to exist as an attribute of EntitySet.

entities is gone as an attribute now and replaced with dataframes

chukarsten · 2021-09-29T17:20:11Z

evalml/tests/component_tests/test_featuretools.py

@@ -144,10 +144,10 @@ def test_ft_woodwork_custom_overrides_returned_by_components(X_df):
        assert isinstance(transformed, pd.DataFrame)
        if logical_type == Datetime:
            assert {k: type(v) for k, v in transformed.ww.logical_types.items()} == {


I think this change is a result of this.

Cool, this makes sense given what these features mean!

Although the naming is kinda weird, like was the original feature name "0"? That must be it

…aturetools 1.0.0 dev branch.

…ools dev branches.

…ew, higher min reqs.

jeremyliweishih

this looks good to me and I'm fine with the del solution as long as we have proper documentation for it!

thehomebrewnerd

Just a couple other minor comments in addition to those by @jeremyliweishih and @freddyaboulton. I'd like to think a bit more about the naming issue during EntitySet creation - deleting the accessor and reinitializing isn't ideal.

core-requirements.txt

evalml/pipelines/components/transformers/preprocessing/text_featurizer.py

evalml/pipelines/components/transformers/preprocessing/transform_primitive_components.py

thehomebrewnerd · 2021-10-14T15:24:46Z

evalml/tests/component_tests/test_featuretools.py

@@ -50,19 +50,19 @@ def test_featuretools_index(mock_calculate_feature_matrix, mock_dfs, X_y_multi):
    feature = DFSTransformer()
    feature.fit(X_new_index)
    feature.transform(X_new_index)


entities is gone as an attribute now and replaced with dataframes

freddyaboulton

Thank you @chukarsten for doing this and thank you the @thehomebrewnerd for the thoughtful review!

Given @thehomebrewnerd 's comment about del X.ww, I think this is good to go. The featuretools component is not currently used by default in AutoMLSearch (for now) so I think we have time to figure out if there's a better approach. The @thehomebrewnerd would you be ok with us filing a ft issue?

thehomebrewnerd · 2021-10-14T16:20:29Z

@thehomebrewnerd would you be ok with us filing a ft issue?

Of course! I think we can add the functionality you want, but might need to take a little time to think through the best way to do it...

chukarsten · 2021-10-14T16:24:09Z

Ok @thehomebrewnerd @freddyaboulton , I'm going to make the minor changes and push. We'll move forward with this and I'll post the issue filed against Featuretools here. Thanks everyone for the great reviews and discussion.

…sor deletion for later removal.

dsherry · 2021-10-14T16:44:26Z

core-requirements.txt

-nlp-primitives>=1.1.0
+woodwork==0.8.1
+dask>=2021.2.0
+nlp-primitives>=1.0.0


Hmm, I'm confused

Old req here was "nlp-primitives>=1.1.0"

New code lowers min to "nlp-primitives>=1.0.0"

Changelog says "nlp-primitives 2.0.0"

Which version of nlp-primitives are we trying to install? If its 2.0.0, shouldn't we say "nlp-primitives>=2.0.0" here?

Otherwise we're telling users they can install nlp-primitives 1.0.0 with ft 1.0.0, and that will be buggy

Yep! You are correct. I think I just got confused. I moved it to nlp-primitives>=2.0.0

dsherry · 2021-10-14T16:44:59Z

evalml/pipelines/components/transformers/preprocessing/featuretools.py

        ft_es = EntitySet()
+        # TODO: This delete was introduced for compatibility with Featuretools 1.0.0.  This should
+        # be removed after Featuretools handles unnamed dataframes being passed to this function.
+        del X.ww


Could you please explain why this is necessary?

The discussion is here

@dsherry Somewhat short version of the discussion - currently Featuretools doesn't allow you to specify values for other arguments if you pass a a woodwork dataframe to EntitySet.add_dataframe. So, if you need to do things like make_index or set the name during the add_dataframe call, you need to first clear out woodwork otherwise FT doesn't work as expected. This can be fixed in the future, but that's how it is for now.

Issue filed here: alteryx/featuretools#1740

But long story short: adding a dataframe to an EntitySet requires either a WW init'd Dataframe with a name and an index, or just a dataframe that's not init'd. It doesn't handle a WW init'd Dataframe without a name. We submitted a request to handle what EvalML frequently sends to this function, a WW init'd df without a name, but until then, we can turn them into regular dataframes and let the infer_feature_types convert it back later.

evalml/tests/dependency_update_check/minimum_core_requirements.txt

dsherry · 2021-10-14T16:47:42Z

evalml/tests/dependency_update_check/minimum_core_requirements.txt

-featuretools==0.26.1
-nlp-primitives==1.1.0
+woodwork==0.8.1
+dask==2021.2.0


Is this a typo?

@dsherry Dask switched their versioning approach a while back, so this is valid.

I don't think so. Dask changed how they do their version numbering. 2.12.0 is pretty old. https://github.com/dask/dask/releases?after=2.14.0

Lol got it thanks

setup.py

dsherry

@chukarsten I found a few things which need updating before this is mergeable, feel free to dismiss my change request if I'm not around

chukarsten · 2021-10-14T18:29:32Z

Ok, I added this issue to featuretools so we can revisit a refactor here.

chukarsten · 2021-10-14T18:32:15Z

Also filed a related EvalML issue #2910 so that we can backlog it and put it into our workflow.

Changes were addressed!

chukarsten force-pushed the feature_tools_upgrade branch from a7ae78b to 31c7d28 Compare September 29, 2021 16:33

chukarsten marked this pull request as ready for review September 29, 2021 16:35

chukarsten force-pushed the feature_tools_upgrade branch from 31c7d28 to c500472 Compare September 29, 2021 16:35

auto-assign bot assigned chukarsten Sep 29, 2021

chukarsten commented Sep 30, 2021

View reviewed changes

chukarsten force-pushed the feature_tools_upgrade branch 2 times, most recently from bffc98d to 57e5626 Compare October 1, 2021 19:03

chukarsten force-pushed the feature_tools_upgrade branch from 8dc4d47 to 17787a9 Compare October 12, 2021 22:08

chukarsten added 19 commits October 14, 2021 10:01

Initial.

3b51eed

Fixed conflict.

2e86cd8

Trying to install from a branch.

57ceb51

All tests should be passing with the nlp-primitives dev branch and fe…

8e9df53

…aturetools 1.0.0 dev branch.

Lint.

768ff70

Updated the core-requirements to pull the nlp primitives and featuret…

6969e67

…ools dev branches.

Release notes.

7562399

Updated min dependencies.

a955174

Updated minimum.

e89b3af

Updated to use nlp_prims RC.

68d5ced

Bumped dask.

28a32a9

Bumped dask.

fdd664a

Really?

e1c0eed

Updated release notes, bumped up the core requirements to match the n…

1916b20

…ew, higher min reqs.

Updated release notes.

e2053d8

Updated version numbers to make this the release candidate.

45cc10c

Updated FT version.

a3c384d

Updated FT and unpinned nlp-prims.

2f32e3c

Again, updated deps.

6f4f7cf

chukarsten force-pushed the feature_tools_upgrade branch from 5d88630 to 6f4f7cf Compare October 14, 2021 14:02

chukarsten requested a review from angela97lin October 14, 2021 14:22

chukarsten requested review from ParthivNaresh, dsherry, eccabay, jeremyliweishih and thehomebrewnerd October 14, 2021 14:22

chukarsten added 2 commits October 14, 2021 10:30

Updated latest versions.

ab23d01

Removed RC stuff.

70f0d30

jeremyliweishih approved these changes Oct 14, 2021

View reviewed changes

thehomebrewnerd reviewed Oct 14, 2021

View reviewed changes

freddyaboulton approved these changes Oct 14, 2021

View reviewed changes

Updated with review comments and commented the controversial ww acces…

d99432b

…sor deletion for later removal.

chukarsten changed the title ~~Upgrade to support Featuretools 1.0.0rc~~ Upgrade to support Featuretools 1.0.0 and nlp-primitives 2.0.0 Oct 14, 2021

dsherry reviewed Oct 14, 2021

View reviewed changes

evalml/tests/dependency_update_check/minimum_core_requirements.txt Outdated Show resolved Hide resolved

dsherry reviewed Oct 14, 2021

View reviewed changes

setup.py Outdated Show resolved Hide resolved

dsherry previously requested changes Oct 14, 2021

View reviewed changes

Addressed comments.

d1f869e

chukarsten requested a review from dsherry October 14, 2021 17:32

Merge branch 'main' into feature_tools_upgrade

988eaaf

chukarsten mentioned this pull request Oct 14, 2021

Refactor how Featuretools 1.0.0 compatibility was handled #2910

Open

chukarsten merged commit 5c85614 into main Oct 14, 2021

chukarsten mentioned this pull request Oct 14, 2021

Release v0.35.0 #2918

Merged

chukarsten deleted the feature_tools_upgrade branch October 14, 2021 19:07

Conversation

chukarsten commented Sep 27, 2021

Uh oh!

codecov bot commented Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

chukarsten Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeremyliweishih left a comment

Choose a reason for hiding this comment

Uh oh!

thehomebrewnerd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freddyaboulton left a comment

Choose a reason for hiding this comment

Uh oh!

thehomebrewnerd commented Oct 14, 2021

Uh oh!

chukarsten commented Oct 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dsherry left a comment

Choose a reason for hiding this comment

Uh oh!

chukarsten commented Oct 14, 2021

Uh oh!

chukarsten commented Oct 14, 2021

Uh oh!

codecov bot commented Sep 29, 2021 •

edited

Loading

chukarsten Sep 29, 2021 •

edited

Loading