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

Preliminary switch to CmdStan backend #21

Closed
wants to merge 13 commits into from
Closed

Preliminary switch to CmdStan backend #21

wants to merge 13 commits into from

Conversation

WardBrian
Copy link
Contributor

@WardBrian WardBrian commented Dec 13, 2021

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

This is related to facebook/prophet#2041. The goal is simply to get a working version of prophet which builds using the CmdStanPy backend.

This builds locally for the linux builds.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@WardBrian
Copy link
Contributor Author

@conda-forge-admin, please rerender

@WardBrian WardBrian changed the title Preliminary switch Preliminary switch to CmdStan backend Dec 13, 2021
@maresb
Copy link
Contributor

maresb commented Dec 13, 2021

Awesome!!! I've been dreaming of this moment for a very long time. 😄

@WardBrian
Copy link
Contributor Author

Huh, just noticed that CmdStan never built on MacOS for 2.26. Seems like the CI failed after passing in the PR and I never re-ran it.

@maresb
Copy link
Contributor

maresb commented Dec 13, 2021

@WardBrian, I am hopeful that this will solve #10 (comment) if we no longer have to rely on the pickled model.

@maresb
Copy link
Contributor

maresb commented Dec 13, 2021

See also #19 and the linked PRs #17 and #18. Something is broken, and I don't think I ever figured out what.

@WardBrian
Copy link
Contributor Author

Yeah I don't think the PyStan recommendation of pickling model objects was ever going to scale very well to this level of userbase

@maresb
Copy link
Contributor

maresb commented Dec 13, 2021

Maybe you can figure it out since you now have so much experience with compilers on conda-forge. 😅

@WardBrian
Copy link
Contributor Author

I knew I should have brought my windows laptop with me today. Anyway, currently this works for Linux and MacOS. For some reason the windows build failed before even running the pip install command. I'm hoping it works this time, if not I'll investigate more on a local machine tomorrow

@WardBrian
Copy link
Contributor Author

@maresb, looking at #10 it is almost certainly caused by the pin_compatible, which is only necessary if you're building against the numpy C api (which prophet isn't, but I think PyStan does). It can be removed, but it might make sense to wait until the prophet build process has been simplified and you don't need all the runtime dependencies at build time.

@maresb
Copy link
Contributor

maresb commented Dec 13, 2021

@WardBrian sounds like a great plan! Thanks so much for working on this.

@WardBrian
Copy link
Contributor Author

The most recent commit here passed all tests. I rebuilt 2.26 for OSX this morning, so I'm trying on that version as it is currently the preferred one for Prophet it seems.

I enabled Azure saving artifacts from these builds but haven't been able to download them for some reason, possibly permissions related. If anyone else can, it would be great to have someone test this on their own machine. Note: conda install from a tar file doesn't properly handle dependencies, so if you want to test it make sure you already have all the runtime dependencies for prophet (minus pystan) installed in that environment.

@maresb maresb mentioned this pull request Dec 13, 2021
5 tasks
@WardBrian
Copy link
Contributor Author

Okay I can confirm on Ubuntu 20.04, Python 3.9 that I was able to exactly recreate the peyton manning example from the Prophet docs using this build.

@WardBrian
Copy link
Contributor Author

@conda-forge-admin, please rerender

@WardBrian
Copy link
Contributor Author

I've additionally tested on Windows 10 and MacOS 11.4 and can confirm the built binary works exactly as intended

Copy link
Contributor

@maresb maresb left a comment

Choose a reason for hiding this comment

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

It seems that Linux and Mac packages are including /bin/cmdstan/stan/src/stan/model/model_header.hpp.gch which is 168 MB. Is this necessary, or is it possible to prevent this?

@maresb
Copy link
Contributor

maresb commented Dec 14, 2021

@bletham, are you okay with us defaulting to CmdStan here on Conda-Forge???

@WardBrian
Copy link
Contributor Author

If we don’t want to merge this right away that’s totally fine by me, it was just important to verify that this would be reasonable to do before dropping PyStan upstream. That said, it is workable with the current version so you could have a new build which relies solely on cmdstan. I’ll try to address the above comments either way

@maresb
Copy link
Contributor

maresb commented Dec 14, 2021

I'm a bit out of the loop in general regarding Prophet. I just now saw the reference to facebook/prophet#2041. I don't have any opinion about when to merge.

Hopefully we can delete the that huge header. But regardless, everything looks great to me! 😄 I'm very glad to see all those crusty aspects of installation being fixed up.

@maresb
Copy link
Contributor

maresb commented Dec 15, 2021

@WardBrian, this looks great to me. I'll leave it to you and @bletham to decide when is the right time to merge.

@WardBrian
Copy link
Contributor Author

@conda-forge-admin, please rerender

@xhochy
Copy link
Member

xhochy commented Jun 25, 2022

@WardBrian Can we merge this? I would like to get osx-arm64 builds for prophet but that would require this to be merged first.

@WardBrian
Copy link
Contributor Author

My plan is to port the changes here into #24 actually, since prophet upstream is now using cmdstanpy by default

@WardBrian WardBrian closed this Jun 27, 2022
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.

4 participants