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 "User Specified Date Feature" #2214

Merged
merged 2 commits into from
Apr 30, 2021

Conversation

ParthivNaresh
Copy link
Contributor

@ParthivNaresh ParthivNaresh commented Apr 30, 2021

Fixes #2215
Reverts #2155

Reverting until we can figure out what's going on with pmdarima during installation

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.

Thanks for keeping main green! Sorry you ran into trouble with conda. We can fix it 😁

@freddyaboulton
Copy link
Contributor

@ParthivNaresh @dsherry What happened?

@ParthivNaresh
Copy link
Contributor Author

@freddyaboulton Filed #2215

@freddyaboulton
Copy link
Contributor

freddyaboulton commented Apr 30, 2021

@ParthivNaresh Why did the "Date Feature" PR add pmdarima as a dependency if the Arima implementation still used statsmodels? I think that's what's causing the conda test failure. pmdarima is listed in our requirements.txt but not in conda recipe (I had suggested we keep pmdarima off the conda recipe since I thought it wasn't needed)

@ParthivNaresh
Copy link
Contributor Author

ParthivNaresh commented Apr 30, 2021

@freddyaboulton We moved ARIMA from the statsmodel implementation to sktime which has pmdarima as a soft dependency. So I guess this means we don't need statsmodel?

@freddyaboulton
Copy link
Contributor

Ah I read the diff wrong! The original date feature pr changed the underlying library used by Arima.

Maybe what we should do then is add pmdarima to the conda recipe? Might need to remove statsmodels from our recipe though. I see pmdarima has a max statsmodels version that's lower than our min.

@codecov
Copy link

codecov bot commented Apr 30, 2021

Codecov Report

Merging #2214 (eac0de7) into main (1ed595e) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             main    #2214     +/-   ##
=========================================
- Coverage   100.0%   100.0%   -0.0%     
=========================================
  Files         296      296             
  Lines       24871    24712    -159     
=========================================
- Hits        24853    24694    -159     
  Misses         18       18             
Impacted Files Coverage Δ
evalml/automl/utils.py 100.0% <ø> (ø)
evalml/preprocessing/utils.py 100.0% <ø> (ø)
.../tests/component_tests/test_datetime_featurizer.py 100.0% <ø> (ø)
evalml/tests/conftest.py 100.0% <ø> (ø)
...s/prediction_explanations_tests/test_explainers.py 100.0% <ø> (ø)
evalml/utils/gen_utils.py 99.6% <ø> (ø)
evalml/automl/automl_search.py 100.0% <100.0%> (ø)
...omponents/estimators/regressors/arima_regressor.py 100.0% <100.0%> (ø)
.../transformers/preprocessing/datetime_featurizer.py 100.0% <100.0%> (ø)
...rmers/preprocessing/delayed_feature_transformer.py 100.0% <100.0%> (ø)
... and 21 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 1ed595e...eac0de7. Read the comment docs.

@ParthivNaresh
Copy link
Contributor Author

@freddyaboulton If statsmodel isn’t required by anything else then I agree, we can take statsmodel out of the recipe. This might also fix the errors I was seeing when I added pmdarima to the recipe. There were a large number of dependency version mismatch issues which may have stemmed from its conflict with statsmodel.

@freddyaboulton
Copy link
Contributor

@ParthivNaresh Yea I don't think statsmodels is used by anything else. I think it's worth seeing if we can patch up the recipe before merging this pr?

@ParthivNaresh ParthivNaresh merged commit 6ef200f into main Apr 30, 2021
@ParthivNaresh
Copy link
Contributor Author

@freddyaboulton Sorry I didn’t see your message before merging (dang refreshing). But I’ll take a look at the recipe next for sure

This was referenced May 4, 2021
@freddyaboulton freddyaboulton deleted the revert-2155-2070-User-Specified-Date-Feature branch May 13, 2022 15:02
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.

Build Conda Package Failure during pmdarima installation
3 participants