Simplify setup.py, make CmdStan the default backend

[WardBrian]

Working wheels (cmdstanpy only): https://github.com/WardBrian/prophet/actions/runs/1855227353

This WIP PR would be the next step for #2041.

Currently it:

1. Makes the repackaging of CmdStan reproducible (doesn't assume an existing installation), and optional (can be disabled with environment variable PROPHET_REPACKAGE_CMDSTAN=false)
2. Checks to see if cmdstan was repackaged before setting the cmdstan path
3. Simplifies setup.py accordingly
4. Makes CMDSTANPY the default backend
5. Removes the distinction between the unix and windows model, as cmdstanpy can compile the "unix" model on all platforms

I'd like to test the current setup and wheel builds, hence this PR and the changes to the github actions files. I am still waiting to hear back from my company about the CLA, so this might sit as a draft until after the holidays.

Still yet to be done:
5. Refactor to remove PyStan entirely

[facebook-github-bot]

Hi @WardBrian!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[tcuongd]

Thanks for starting this Brian! Happy to help where I can. I've just merged in some changes to setup.py: #2089 that allow building on Windows using cmdstanpy==0.9.68. I'm conscious that we can simplify + refactor setup.py quite a bit in this PR, but at least it gives us a working example to compare against.

[WardBrian]

@tcuongd I've merged to try to maintain the changes in that PR, but alongside these repackaging changes.

Have you tested that it is not necessary to have the toolchain installed on end-user's machines? I'm not sure how much is statically linked

[WardBrian]

@tcuongd are you able to allow workflow runs for this PR so I can continue testing?

[tcuongd]

Have you tested that it is not necessary to have the toolchain installed on end-user's machines? I'm not sure how much is statically linked

Hmm are you specifically talking about the Rtools toolchain on Windows, or more generally for Linux and MacOS as well?

In the wheel.yml workflow, we:

• Build the package in an isolated environment
• Then install the built wheel into the global python environment, and run the tests in the tests/ folder.

The second step would just use all the pre-packaged code, so I think if there's any issues these tests would fail. Would definitely appreciate a second look at this to confirm though.

The MacOS and Linux packages seem to work fine testing on my machines, but I'm not sure whether the Windows package would work for someone who doesn't have Rtools installed. We'd want to call out that prerequisite pretty clearly, although I don't think it needs to be automated as a part of the installation.

[WardBrian]

Yeah, specifically I wonder if someone on windows needs to have a Windows-compatible gcc/glibc. I can try to check once I get these wheels building

[WardBrian]

@tcuongd - I’m not very familiar with the test suite, could you help figure out why this test is failing? https://github.com/facebook/prophet/runs/4768342262?check_suite_focus=true

Otherwise we passed on Ubuntu. It looks like at least macOS failed because of a GitHub rate limit downloading cmdstan

[tcuongd]

could you help figure out why this test is failing?

Yeah sure I can take a look. Really weird that it's failing though if we haven't changed any other code.

GitHub rate limit downloading cmdstan

Ah yeah, I remember now, this is why I opted to download cmdstan via wget - it's much less fickle and faster as well.

[WardBrian]

For me, the same test fails on main

[WardBrian]

@tcuongd I'm happy to continue working on this if you get a chance to debug that test

[akosfurton]

@tcuongd or @WardBrian, have either of you had a chance to investigate the failing tests?

[WardBrian]

I have not looked into them besides confirming they occur on main if I use the cmdstanpy backend.

[malmashhadani-88]

For the Python 3.7 tests, the specific test test_diagnostics.test_cross_validation fails

The failure is due to this line:

params = self.stan_to_dict_numpy(
self.stan_fit.column_names,
self.stan_fit.optimized_params_np
)

which fails because the values in these variables are incorrectly parsed from the cmdstan CSV output.

The cmdstan CSV output looks like it did not output completely correctly. It is missing a line-break after the parameter names and their values.

The CSV has both the parameter names and values on the same line, which is not the format the cmdstanpy expects.

Sample CSV attached below:
prophet_model-20220211103525.csv

Environment:

• Python 3.7
• Test: test_diagnostics.test_cross_validation
• Test only fails on method=Newton and fails when the cross validation is parallelized using ThreadPoolExecutor or ProcessPoolExecutor
  • Possibly a race condition with the csv file names only being unique to the second?

@WardBrian , since you are also a contributor to cmdstanpy, could you help with the CSV parsing?

[WardBrian]

This is the kind of error we have seen when multiple cmdstan processes are trying to write to the same file at the same time, usually in threaded environments where the chain ID is not being explicitly set. Does that sound like a plausible explanation for this error @malmashhadani-88 ?

[akosfurton]

@WardBrian , exactly as hypothesized. Could you please re-run all tests with this additional change?

The test is running multiple models in parallel (cross-validating the time series), so all of the output files were being written to the same location.

By adding output_dir = mkdtemp(), to the fit function of models.py, all tests appear to pass.

[WardBrian]

Glad it is a simple fix. Could you be more specific about what I should change in the test?

[ShantanuKumar]

Hi, I tried to use these changes to train model using prophet but I get this error about missing libtbb.dylib.
I can install prophet though. This happens both on macos and inside a docker container.

[WardBrian]

Can you share the error?

[ShantanuKumar]

This is generated on macos. I was able to fix it inside a docker container with linux by manually installing tbb. I tried to do the same on macos using brew install tbb but that doesn't work. brew does install tbb successfully. It's when I try to train a model, I get this error. I am on macos Monterey.

INFO:prophet:Disabling yearly seasonality. Run prophet with yearly_seasonality=True to override this.
INFO:prophet:Disabling daily seasonality. Run prophet with daily_seasonality=True to override this.
INFO:cmdstanpy:start chain 1
INFO:cmdstanpy:finish chain 1
WARNING:prophet.models:Optimization terminated abnormally. Falling back to Newton.
INFO:cmdstanpy:start chain 1
INFO:cmdstanpy:finish chain 1
Traceback (most recent call last):
  File "/Users/developer/.pyenv/versions/abb-crc-env/lib/python3.9/site-packages/prophet/models.py", line 105, in fit
    self.stan_fit = self.model.optimize(**args)
  File "/Users/developer/.pyenv/versions/abb-crc-env/lib/python3.9/site-packages/cmdstanpy/model.py", line 435, in optimize
    raise RuntimeError(msg)
RuntimeError: Error during optimization.
RunSet: chains=1
 cmd:
        ['/Users/developer/.pyenv/versions/3.9.0/envs/abb-crc-env/lib/python3.9/site-packages/prophet/stan_model/prophet_model.bin', 'random', 'seed=36598', 'data', 'file=/var/folders/9w/9_4n35r57d707zkrk86rvh9w0000gn/T/tmpffcs4eo7/j7fjjyax.json', 'init=/var/folders/9w/9_4n35r57d707zkrk86rvh9w0000gn/T/tmpffcs4eo7/rs05hk_o.json', 'output', 'file=/private/var/folders/9w/9_4n35r57d707zkrk86rvh9w0000gn/T/tmpxll2ozny/prophet_model-202203180948-1.csv', 'method=optimize', 'algorithm=lbfgs', 'iter=10000']
 retcodes=[-6]
 error_msgs:
        /private/var/folders/9w/9_4n35r57d707zkrk86rvh9w0000gn/T/tmpxll2ozny/prophet_model-202203180948-1-stderr.txt
chain_id 1:
dyld[30142]: Library not loaded: @rpath/libtbb.dylib
  Referenced from: /Users/developer/.pyenv/versions/3.9.0/envs/abb-crc-env/lib/python3.9/site-packages/prophet/stan_model/prophet_model.bin
  Reason: tried: '/private/var/folders/9w/9_4n35r57d707zkrk86rvh9w0000gn/T/pip-install-oqo49qox/prophet_deddf0531f6c41cf8980f3b9563b7023/python/build/lib.macosx-11.5-x86_64-3.9/prophet/stan_model/cmdstan-2.26.1/stan/lib/stan_math/lib/tbb/libtbb.dylib' (no such file), '/private/var/folders/9w/9_4n35r57d707zkrk86rvh9w0000gn/T/pip-install-oqo49qox/prophet_deddf0531f6c41cf8980f3b9563b7023/python/build/lib.macosx-11.5-x86_64-3.9/prophet/stan_model/cmdstan-2.26.1/stan/lib/stan_math/lib/tbb/libtbb.dylib' (no such file), '/usr/local/lib/libtbb.dylib' (no such file), '/usr/lib/libtbb.dylib' (no such file)

[WardBrian]

I will investigate this. In the mean time, you may be able to use brew install tbb@2020 to get it working

[WardBrian]

@ShantanuKumar were you using the downloads from the link I posted above (https://github.com/WardBrian/prophet/actions/runs/1855227353)?

[ShantanuKumar]

@WardBrian I built the library locally from the commit without pystan on your forked branch.

pip install git+https://github.com/WardBrian/prophet.git@18f19f7f989bc38489a1601fa111193f2772dce1#subdirectory=python

[WardBrian]

Are you able to download the correct wheel from that GithubAction run and try that?

[ShantanuKumar]

I only see wheels for 3.8, but I am using python 3.9

[WardBrian]

If you download the zip file you will find wheels for many versions

[ShantanuKumar]

That worked!

[ShantanuKumar]

Can we please merge this to main?

[akosfurton]

@bletham or @tcuongd , can you please merge this? I believe you are the only two with write access

[tcuongd]

Hmm not sure why CI hasn't been running for the more recent pushes, it ran here: https://github.com/facebook/prophet/actions/runs/1853320459
I'd like to kick off another CI check before merging; @WardBrian would you be able to give me push access to your fork? I can usually get CI working again by making a push from my account

[WardBrian]

@tcuongd I’ve just checked the “allow access by maintainers” box so you should be able to

[tcuongd]

Amazing stuff!!!

• Great to see that cmdstanpy.install_cmdstan() is fast now and we don't need to use wget; makes the wheel build workflow so much cleaner.
• The refactor to setup.py looks all good to me, again much cleaner when we make use of the cmdstanpy internals.
• Thanks so much to everyone for figuring out tricky bugs like the interaction between multiprocessing and writing to csvs, and the Windows TBB stuff.

Happy to merge this PR but please keep in mind:

• I'm unable to release a new package version to PyPI, so for now Prophet on PyPI will remain at 1.0.1, with all of the pystan installation code, and will only support Python 3.6-3.8.
• The changes in this PR (removing pystan, support for Python 3.9+) will be accessible by cloning the Prophet repo and building the package manually from source (this is effectively the 'development version', which I've set to 1.1). I've added a section for how to do this in the README. (ps: @WardBrian could you have a read of these changes and let me know if it makes sense)
• If you don't want to build from source, but want to access the changes in this PR, please download the latest wheels here: https://github.com/WardBrian/prophet/actions/runs/1855227353 -- the downloaded wheel files can be installed using pip.

I understand this is confusing but 🤞 we can get a PyPI release soon (then everyone will actually be able to "just pip install prophet", heh).

[thomasaarholt]

(been following this PR with quite some interest)
What is the blocker for a PyPi release?

[tcuongd]

(been following this PR with quite some interest) What is the blocker for a PyPi release?

I think Ben (one of the package authors) has the release process set up on his machine, but I haven't been able to get in touch with him recently.

[WardBrian]

@tcuongd the README changes look good to me. I'm actually not 100% sure if the final section ("Using cmdstanpy with Windows requires a Unix-compatible C compiler such as mingw-gcc.") is true with the pre-compiled installations from wheels, but it certainly is if building from source.

Shame about the PyPi releases. This is part of why on CmdStanPy we do releases through and action

[tcuongd]

@tcuongd the README changes look good to me. I'm actually not 100% sure if the final section ("Using cmdstanpy with Windows requires a Unix-compatible C compiler such as mingw-gcc.") is true with the pre-compiled installations from wheels, but it certainly is if building from source.

Shame about the PyPi releases. This is part of why on CmdStanPy we do releases through and action

I definitely want to have this workflow too! Will bring it up with the authors when I get the chance.