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

fix(dependency): Pin pystan after breaking prophet install #13852

Merged
merged 1 commit into from
Apr 1, 2021

Conversation

EBoisseauSierra
Copy link
Contributor

SUMMARY

Prophet is heavily dependent on Pystan: https://facebook.github.io/prophet/docs/installation.html

Pystan has recently (2021-03-25) release its v3.0.0.

This release is not backward compatible and breaks fbprophet: facebook/prophet#1856

(Indeed, fbprophet 0.6 specifies pystan>=2.14 — but doesn't restrict upgrading to next major release: https://github.com/facebook/prophet/blob/0.6/python/requirements.txt#L3)

So we need to pin pystan to the lastest non 3.x release, while fbprophet fixes the dependency on their side.

TEST PLAN

pip install '.[prophet]'?

ADDITIONAL INFORMATION

@EBoisseauSierra
Copy link
Contributor Author

Note: fbprophet's team said 3 days ago:

We're pushing a new release with the updated pinned version very soon.

@junlincc junlincc requested a review from villebro March 29, 2021 18:27
@junlincc
Copy link
Member

Thanks Étienne for surfacing it! so much going on we lost track of updating/maintaining this feature. @EBoisseauSierra would you like to 'own' this piece and keep it in good shape going forward? 😉

@codecov
Copy link

codecov bot commented Mar 29, 2021

Codecov Report

Merging #13852 (a73bdb5) into master (467848d) will increase coverage by 1.73%.
The diff coverage is 69.69%.

❗ Current head a73bdb5 differs from pull request most recent head 83ed5fd. Consider uploading reports for the commit 83ed5fd to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13852      +/-   ##
==========================================
+ Coverage   75.56%   77.30%   +1.73%     
==========================================
  Files         935      938       +3     
  Lines       47275    47558     +283     
  Branches     5883     6005     +122     
==========================================
+ Hits        35725    36765    +1040     
+ Misses      11375    10646     -729     
+ Partials      175      147      -28     
Flag Coverage Δ
cypress 56.02% <47.50%> (+8.32%) ⬆️
hive 80.29% <70.00%> (+<0.01%) ⬆️
mysql 80.60% <70.00%> (+<0.01%) ⬆️
postgres 80.63% <70.00%> (+<0.01%) ⬆️
presto 80.29% <70.00%> (-0.01%) ⬇️
python 81.14% <70.00%> (-0.01%) ⬇️
sqlite 80.19% <70.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset-frontend/src/views/CRUD/alert/types.ts 100.00% <ø> (ø)
superset/reports/logs/api.py 95.45% <ø> (ø)
superset/tasks/scheduler.py 0.00% <0.00%> (ø)
superset/views/core.py 75.66% <0.00%> (ø)
...veFilters/FilterBar/FilterControls/FilterValue.tsx 73.03% <50.00%> (+0.81%) ⬆️
...erset-frontend/src/SqlLab/components/ResultSet.tsx 47.12% <60.00%> (-21.82%) ⬇️
...ters/FiltersConfigModal/FiltersConfigForm/state.ts 75.92% <69.23%> (+23.29%) ⬆️
...tersConfigModal/FiltersConfigForm/DefaultValue.tsx 80.55% <100.00%> (+4.69%) ⬆️
...onfigModal/FiltersConfigForm/FiltersConfigForm.tsx 94.18% <100.00%> (+6.68%) ⬆️
.../src/explore/components/controls/SelectControl.jsx 93.75% <100.00%> (+4.46%) ⬆️
... and 127 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 467848d...83ed5fd. Read the comment docs.

@etr2460
Copy link
Member

etr2460 commented Mar 30, 2021

will prophet 1.0.1 work with Superset? if not, then this change makes sense. otherwise, let's bump prophet instead

@EBoisseauSierra
Copy link
Contributor Author

EBoisseauSierra commented Mar 30, 2021

will prophet 1.0.1 work with Superset?

You are right that it's a better thing to do.

Unfortunately, I hadn't much time to extensively test bumping fbprophet<0.7 to prophet>=1.01, so — especially as I'm not so familiar with the codebase and that I couldn't find in 7af8b2b3f why was the <0.7 constraint applied — I preferred shipping a quick fix first.

I will test bumping [fb]prophet instead, but if it passes test, I'd still appreciate some external sanity check as I couldn't do much more than blindly relying on them.

P.S.: FYI, I'm currently testing it on https://github.com/EBoisseauSierra/superset/tree/bump-prophet

would you like to 'own' this piece and keep it in good shape going forward?

Thanks for offering. I would be happy to monitor when Prophet fixes its dependency, to revert this change; yet I'm afraid I am not familiar enough with prophet to properly “own” the extension.

@EBoisseauSierra EBoisseauSierra changed the title [WIP] Pin pystan to working release with prophet [WIP] fix(dependency): Pin pystan after breaking prophet install Mar 30, 2021
@EBoisseauSierra EBoisseauSierra marked this pull request as draft March 30, 2021 11:39
@EBoisseauSierra EBoisseauSierra changed the title [WIP] fix(dependency): Pin pystan after breaking prophet install fix(dependency): Pin pystan after breaking prophet install Mar 30, 2021
Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

This lgtm, thanks for pulling in the dep pinning patch from upstream

Copy link
Member

@villebro villebro 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 fixing this - I noticed the other day I had trouble getting prophet to work but didn't have time to investigate, I'm happy I was able to freeride on your fix! :)

setup.py Outdated Show resolved Hide resolved
Prophet is heavily dependent on Pystan:
https://facebook.github.io/prophet/docs/installation.html

Pystan has recently (2021-03-25) release its v3.0.0.

This release is not backward compatible and breaks fbprophet:
facebook/prophet#1856

(Indeed, fbprophet 0.6 specifies `pystan>=2.14` — but doesn't restrict
upgrading to next major release:
https://github.com/facebook/prophet/blob/0.6/python/requirements.txt#L3)

So we need to pin pystan to the lastest non 3.x release, while fbprophet
fixes the dependency on their side.

We have taken this opportunity to bump fbprophet too to its next minor
version.

Signed-off-by: Étienne Boisseau-Sierra <etienne.boisseau-sierra@unipart.io>
@EBoisseauSierra EBoisseauSierra marked this pull request as ready for review March 30, 2021 16:14
@junlincc junlincc added the need:merge The PR is ready to be merged label Mar 31, 2021
@zhaoyongjie zhaoyongjie merged commit 73a2cc3 into apache:master Apr 1, 2021
amitmiran137 pushed a commit that referenced this pull request Apr 2, 2021
* master: (26 commits)
  chore: bump to new superset-ui version (#13932)
  fix: do not run containers as root by default in Helm chart (#13917)
  feat(explore): adhoc column formatting for Table chart (#13758)
  fix(sqla-query): order by aggregations in Presto and Hive (#13739)
  feat(alert/report): add ALERTS_ATTACH_REPORTS feature flags + feature (#13894)
  test: Fixes PropertiesModal_spec (#13548)
  fix: Pin Prophet dependency after breaking changes (#13852)
  test: Adds tests to dnd controls (#13650)
  test: Adds tests to the AnnotationLayer component (#13748)
  test: Refactor and enhance tests for the Explore DatasourcePanel Component (#13799)
  Add tests (#13778)
  test: DisplayQueryButton (#13750)
  Fixing condition around left margin for dashboard layout. Fixes #13863 (#13905)
  Revert "fix: select table overlay (#13694)" (#13901)
  test: Adds tests to the OptionControls component (#13729)
  test: DatasourceControl (#13605)
  tests for function handleScroll (#13896)
  test: Adds tests to the CustomFrame component (#13675)
  test: Adds tests to the AdvancedFrame component (#13664)
  test: DataTableControl (#13668)
  ...
lyndsiWilliams pushed a commit to preset-io/superset that referenced this pull request Apr 7, 2021
Prophet is heavily dependent on Pystan:
https://facebook.github.io/prophet/docs/installation.html

Pystan has recently (2021-03-25) release its v3.0.0.

This release is not backward compatible and breaks fbprophet:
facebook/prophet#1856

(Indeed, fbprophet 0.6 specifies `pystan>=2.14` — but doesn't restrict
upgrading to next major release:
https://github.com/facebook/prophet/blob/0.6/python/requirements.txt#L3)

So we need to pin pystan to the lastest non 3.x release, while fbprophet
fixes the dependency on their side.

We have taken this opportunity to bump fbprophet too to its next minor
version.

Signed-off-by: Étienne Boisseau-Sierra <etienne.boisseau-sierra@unipart.io>
EBoisseauSierra added a commit to EBoisseauSierra/superset that referenced this pull request Apr 8, 2021
Superset relies on the `fbprophet` package for timeseries forecasting.
Prophet itself relies on `pystan`.

Pystan has released a new MAJOR version (3.x), which is backwards
incompatible. So this breaks the current `fbprophet>=0.6,<0.7`
requirement.

One approach to fix this is to pin the `pystan` (sub)dependency to a
pre-v3.x version (cf. PR apache#13852).

Yet a better approach seems to bump `fbprophet` itself to a pystan>=3.0
compatible version.

In the meantime, `fbprophet` v0.x has been promoted to `prophet` v1.x.
cf. https://pypi.org/project/fbprophet/#history (latest version 0.7.1 on
Sep 5, 2020) vs. https://pypi.org/project/prophet/#history (1.0.1 on Mar
29, 2021).

Signed-off-by: Étienne Boisseau-Sierra <etienne.boisseau-sierra@unipart.io>
EBoisseauSierra added a commit to EBoisseauSierra/puppet-superset that referenced this pull request Apr 15, 2021
See apache/superset#13852

Signed-off-by: Étienne Boisseau-Sierra <etienne.boisseau-sierra@unipart.io>
allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021
Prophet is heavily dependent on Pystan:
https://facebook.github.io/prophet/docs/installation.html

Pystan has recently (2021-03-25) release its v3.0.0.

This release is not backward compatible and breaks fbprophet:
facebook/prophet#1856

(Indeed, fbprophet 0.6 specifies `pystan>=2.14` — but doesn't restrict
upgrading to next major release:
https://github.com/facebook/prophet/blob/0.6/python/requirements.txt#L3)

So we need to pin pystan to the lastest non 3.x release, while fbprophet
fixes the dependency on their side.

We have taken this opportunity to bump fbprophet too to its next minor
version.

Signed-off-by: Étienne Boisseau-Sierra <etienne.boisseau-sierra@unipart.io>
@EBoisseauSierra EBoisseauSierra deleted the fix-pystan-dependency branch September 2, 2021 14:23
EBoisseauSierra added a commit to EBoisseauSierra/puppet-superset that referenced this pull request Sep 2, 2021
as this has been fixed upstream in apache/superset#13852

Signed-off-by: Étienne Boisseau-Sierra <etienne.boisseau-sierra@unipart.io>
EBoisseauSierra added a commit to EBoisseauSierra/puppet-superset that referenced this pull request Sep 2, 2021
as this has been fixed upstream in apache/superset#13852

Signed-off-by: Étienne Boisseau-Sierra <etienne.boisseau-sierra@unipart.io>
@geido geido added viz:charts:echarts Related to Echarts and removed viz:echarts-prophet labels Feb 9, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels need:merge The PR is ready to be merged size/XS viz:charts:echarts Related to Echarts 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pip install '.[prophet]' is broken, due to pystan
7 participants