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

reintegrate prophet in core deps #1054

Merged
merged 7 commits into from
Jul 12, 2022
Merged

reintegrate prophet in core deps #1054

merged 7 commits into from
Jul 12, 2022

Conversation

hrzn
Copy link
Contributor

@hrzn hrzn commented Jul 1, 2022

An attempt to (maybe) reintegrate Prophet (with version 1.1)

@codecov-commenter
Copy link

Codecov Report

Merging #1054 (d5bf7ee) into master (12c3702) will increase coverage by 1.69%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1054      +/-   ##
==========================================
+ Coverage   91.41%   93.11%   +1.69%     
==========================================
  Files          78       78              
  Lines        7955     7941      -14     
==========================================
+ Hits         7272     7394     +122     
+ Misses        683      547     -136     
Impacted Files Coverage Δ
darts/models/__init__.py 81.81% <0.00%> (-4.85%) ⬇️
...arts/models/forecasting/torch_forecasting_model.py 89.51% <0.00%> (-0.05%) ⬇️
darts/models/forecasting/block_rnn_model.py 98.14% <0.00%> (-0.04%) ⬇️
darts/models/forecasting/nhits.py 99.25% <0.00%> (-0.01%) ⬇️
darts/datasets/__init__.py 100.00% <0.00%> (ø)
darts/timeseries.py 92.14% <0.00%> (+0.03%) ⬆️
darts/logging.py 98.14% <0.00%> (+22.22%) ⬆️
darts/models/forecasting/prophet_model.py 94.24% <0.00%> (+89.92%) ⬆️

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 12c3702...d5bf7ee. Read the comment docs.

Copy link
Contributor

@tomasvanpottelbergh tomasvanpottelbergh left a comment

Choose a reason for hiding this comment

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

Looks good! We shouldn't forget to update the docs at the next release.

@hrzn
Copy link
Contributor Author

hrzn commented Jul 11, 2022

I ran some tests with conda envs, Py39:

  • Base environment size: 114 MB
  • Size of u8darts core install, without Prophet: 984 MB
  • Size of u8darts core install, with prophet==1.1: 1019 MB
  • Size of u8darts pmdarima install, with prophet==1.1: 1022 MB
  • Size of full install without Prophet: 1.5 GB
  • Size of full install with prophet==1.1: 1.5 GB

Other observation: pmdarima depends on Cython, which is also a dependency of Prophet. It doesn't seem to have any other potentially problematic dependency, and installed without issue with pip in my tests.

Conclusions:

  • It's safe to re-integrate Prophet as its size doesn't inflate that of Darts too much
  • It's probably safe to drop our pmdarima flavour and just put pmdarima in the core dependencies, and maintain only one flavour for PyTorch. @tomasvanpottelbergh thoughts?

@tomasvanpottelbergh
Copy link
Contributor

tomasvanpottelbergh commented Jul 11, 2022

  • It's safe to re-integrate Prophet as its size doesn't inflate that of Darts too much

👍

  • It's probably safe to drop our pmdarima flavour and just put pmdarima in the core dependencies, and maintain only one flavour for PyTorch. @tomasvanpottelbergh thoughts?

Sounds good as it makes things simpler!

@hrzn hrzn merged commit 7ffa3fc into master Jul 12, 2022
@madtoinou madtoinou deleted the feat/reintegrate-prophet branch July 5, 2023 21:57
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.

3 participants