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

[Draft] Python Wheels for PyPi #2010

Merged
merged 47 commits into from
Oct 3, 2021
Merged

[Draft] Python Wheels for PyPi #2010

merged 47 commits into from
Oct 3, 2021

Conversation

tcuongd
Copy link
Collaborator

@tcuongd tcuongd commented Aug 28, 2021

Having a go at creating wheels for the Python package with the aim of publishing to PyPi. We're trying to create a wheel that has both pystan and cmdstanpy models compiled, so the end user just has to install the wheel and prophet should work out of the box.

Tasks

  • Install cmdstan as a part of setup.py, and include the core cmdstan files in the prophet distribution as well. This allows end users to fit models on the CMDSTANPY backend without an existing installation of cmdstan, and ensures that we the model is fit using the same version of cmdstan that compiled it.
  • Use pyproject.toml to specify build dependencies, with the aim of using pypa/build as the build engine. This seems to be the recommended approach to building wheels.
  • Remove imports of prophet.models from the setup.py script -- the build system was complaining that packages could not be found (because the build happens in an isolated environment). I think this is actually a bit cleaner.
  • Create a wheel.yml Github Actions workflow. The aim is to run this when we release the package, and publish to PyPi automatically. Currently using cibuildwheel to try and simplify this process.
  • Get all relevant wheels built successfully via wheel.yml (across different platforms and for Python versions 3.6, 3.7, 3.8).
  • Finalise the testing + automated publishing via wheel.yml
  • Spot check that building from source on the end user's machines is not broken (would be required for versions / platforms where we haven't published a wheel).

Open questions

Windows

Getting the necessary build tools on Windows will be tricky, but I think cmdstanpy might make it easier with the install_cxx_toolchain utility function. This should be its own PR though.

cmdstan

  • The PR as it currently stands removes the option for the user to use their own cmdstan installation. Should we still make this available for end-users who want to build their own wheel?

@tcuongd tcuongd changed the title [Draft] Python Wheels [Draft] Python Wheels for PyPi Aug 29, 2021
Comment on lines 4 to 7
push:
branches: [ master ]
pull_request:
branches: [ master ]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Temporary, for testing.


def get_backends_from_env() -> List[str]:
from prophet.models import StanBackendEnum
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was causing the build system to fail, since the build happens in an isolated environment with only the dependencies needed for setup.py. I think removing this makes things a bit cleaner, but keen to hear thoughts.

python/setup.py Outdated Show resolved Hide resolved
python/setup.py Outdated Show resolved Hide resolved
@@ -129,15 +181,15 @@ def with_project_on_sys_path(self, func):
author_email='sjtz@pm.me',
license='MIT',
packages=find_packages(),
setup_requires=[
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ensures that we read from pyproject.toml for build dependencies instead.

@tcuongd tcuongd mentioned this pull request Aug 29, 2021
@freddyaboulton
Copy link

@tcuongd Thanks for starting this! I think this is really cool.

I tried some stuff out on my fork: freddyaboulton#1 to no avail as well.

I think the problem with auditwheel is that it's finding compiled code in the wheel when the package does not specify any extensions in the setup.py so it thinks it's "pure".

We can obviously skip the audit step but then the problem is that cmdstan dynamically links against some libraries that would not be included in the wheel so it's actually not exportable.

One thing that came to mind is that if cmdstan statically linked against its dependencies, then we could skip the audit step and I think it should "work". So maybe that's something worth trying?

I'm pretty new to this myself so I don't have any concrete answer (and it's possible what I'm saying here doesn't make sense at all!) but just want to let you know that this is cool and I'm willing to help if possible.

Another point to discuss is that I added testing to the cibuildwheel script and the macosx build passes but then the tests start failing in a really weird way?

https://github.com/freddyaboulton/prophet/runs/3498406868

@tcuongd
Copy link
Collaborator Author

tcuongd commented Sep 4, 2021

Thanks heaps for looking into this as well @freddyaboulton !! Yeah definitely a new area for me too so still trying to piece everything together 😅

Linux wheel

Yeah the Linux one is weird. I did try to bundle the necessary cmdstan files into the wheel without running the repair command, but the wheel was rejected by PyPi when I tried to upload it. I've received some good feedback about this here: pypa/packaging-problems#542 - seems like we'll need to change our setup.py file.

MacOS tests

Oh I didn't realise cibuildwheel had wheel tests baked in as well, nice find! I think what's happening on your branch is that it's trying to run the tests against the source code ({project}/tests), but that's not where we built the binaries. 🤞 if we change that to {package}/tests it'll work.

Keep me updated! I'll keep trying to get Linux working as well.

@tcuongd tcuongd linked an issue Sep 5, 2021 that may be closed by this pull request
@freddyaboulton
Copy link

@tcuongd great call adding the custom build_ext command to setup.py! I was able to fix the unit test issue and add a couple of cmdstanpy tests to verify the built wheels would pass the unit tests for both back ends as expected! I opened up those changes as a PR to this branch.

@freddyaboulton
Copy link

@tcuongd this looks ready to merge for me. What do you think?

@tcuongd
Copy link
Collaborator Author

tcuongd commented Oct 2, 2021

@freddyaboulton Thanks for the reminder! Totally dropped the ball on this one 😅 Yeah it's probably a good idea to merge some of this into master, so it's easier to extend and test before we do a release.

Going to work on cleaning this up over the next 2 days.

@tcuongd tcuongd marked this pull request as ready for review October 3, 2021 00:59
@tcuongd
Copy link
Collaborator Author

tcuongd commented Oct 3, 2021

Alrighty I will merge this into master now since it's passing the regular build-and-test CI, and we can iterate on it. I've also set the wheel workflow runs to be triggered on release create and workflow_dispatch (i.e. manual runs). Since the manual run only becomes trigger-able after this is merged into master, I'll do another check that nothing's broken after the merge.

@tcuongd tcuongd merged commit 4fb577e into master Oct 3, 2021
@tcuongd tcuongd deleted the tcuongd-cmdstanpy-wheel branch October 3, 2021 01:03
@freddyaboulton
Copy link

@tcuongd When is this being released?

@octavd
Copy link

octavd commented Feb 9, 2022

@tcuongd looking forward for this also.

@lmmentel
Copy link

Awesome! Looking forward to it. It'll have a lot of impact reducing the install times 🙌

@akosfurton
Copy link

I believe this PR + a follow up PR to remove the PyStan backend should unblock #2041

Then, prophet 1.1 can be released to PyPi #2064

@freddyaboulton
Copy link

freddyaboulton commented Mar 23, 2022

@tcuongd @akosfurton Any update on when prophet 1.1 will be released? Thank you!

@akosfurton
Copy link

@tcuongd @akosfurton Any update on when prophet 1.1 will be released? Thank you!

I don't have write access to merge the PR, only @tcuongd and @bletham do. Will try to contact them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upload wheel to PyPI
6 participants