-
Notifications
You must be signed in to change notification settings - Fork 4.6k
[Draft] Python Wheels for PyPi #2010
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
Conversation
.github/workflows/wheel.yml
Outdated
| push: | ||
| branches: [ master ] | ||
| pull_request: | ||
| branches: [ master ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporary, for testing.
| CMDSTAN_VERSION = "2.26.1" | ||
|
|
||
| def get_backends_from_env() -> List[str]: | ||
| from prophet.models import StanBackendEnum |
There was a problem hiding this comment.
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.
| author_email='sjtz@pm.me', | ||
| license='MIT', | ||
| packages=find_packages(), | ||
| setup_requires=[ |
There was a problem hiding this comment.
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 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? |
|
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 MacOS tests Oh I didn't realise Keep me updated! I'll keep trying to get Linux working as well. |
|
@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. |
|
@tcuongd this looks ready to merge for me. What do you think? |
|
@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. |
…/prophet into tcuongd-cmdstanpy-wheel
|
Alrighty I will merge this into |
|
@tcuongd When is this being released? |
|
@tcuongd looking forward for this also. |
|
Awesome! Looking forward to it. It'll have a lot of impact reducing the install times 🙌 |
|
@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. |
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
pystanandcmdstanpymodels compiled, so the end user just has to install the wheel andprophetshould work out of the box.Tasks
cmdstanas a part ofsetup.py, and include the corecmdstanfiles in the prophet distribution as well. This allows end users to fit models on theCMDSTANPYbackend without an existing installation of cmdstan, and ensures that we the model is fit using the same version of cmdstan that compiled it.pyproject.tomlto specify build dependencies, with the aim of usingpypa/buildas the build engine. This seems to be the recommended approach to building wheels.prophet.modelsfrom thesetup.pyscript -- 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.wheel.ymlGithub 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.wheel.yml(across different platforms and for Python versions 3.6, 3.7, 3.8).auditwheelto create themanylinuxwheels.wheel.ymlOpen questions
Windows
Getting the necessary build tools on Windows will be tricky, but I think
cmdstanpymight make it easier with theinstall_cxx_toolchainutility function. This should be its own PR though.cmdstan
cmdstaninstallation. Should we still make this available for end-users who want to build their own wheel?