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

Upgrade python version #49

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

pperanich
Copy link

Description

This PR aims to address the following:

  • Labgraph build support for CPython3.6-3.10
  • Initial pass at standing up a cibuildwheel Github Action for building labgraph for platforms on push and pull-request.
  • Upgrading dependencies in labgraph core and extensions to provide support for python3.6-3.10
  • Updated install instructions for extensions
  • Fixed breaking changes in labgraph core and extensions for new python versions.

Fixes #(issue)
#34

Type of change

  • [ x ] New feature (non-breaking change which adds functionality)
  • [ x ] This change requires a documentation update

Feature/Issue validation/testing

Dockerfile updates

To support compiling for python version after 3.6, an additional -fPIC position independent code flag was required in compiling python for the manylinux2014 image. A pull request is currently open (pypa/manylinux#1258) to merge this flag change into the manylinux repo. For testing, the manylinux2014 was compiled on my local machine, followed by a docker build ., which would then use the local manylinux2014 image. The default Dockerfile now compiles a manylinux compatible wheel for python 3.6-3.10.

cibuildwheel progress

A cibuildwheel action has been created to build Labgraph wheels for Mac, Windows, and Linux

  • The Linux build job should work once Added position independent code flag for python compilation. pypa/manylinux#1258 is closed and the latest images are deployed.
  • The Mac build job currently installs all required tools and is able to successfully compile the Py3.6 wheel, but fails on the tests that follow with a segmentation fault. Not sure of the source of this issue, but it could be related to the MacOS repair wheel command not currently having access to DYLD_LIBRARY_PATH (https://cibuildwheel.readthedocs.io/en/stable/faq/#macos-passing-dyld_library_path-to-delocate).
  • The Windows build job installs the required tools, but its environment variables are not currently set correctly in the build step. Mspatz/docker windows #33 provides a Dockerfile.Windows which is able to successfully compile labgraph in a container, and it sets the environment variables with the following:
    RUN C:\Program^ Files^ ^(x86^)\Microsoft^ Visual^ Studio\2019\BuildTools\VC\Auxiliary\Build\vcvars64.bat && python setup_py36.py build
    Though I call vcvars64.bat in scripts/prepare_build_for_windows.bat, the build job fails to set the environment correctly. You may be able to make use of CIBW_ENVIRONMENT (https://cibuildwheel.readthedocs.io/en/stable/options/#environment) to inject the correct variables into the build environment, though I have failed to find a way to execute the command and set the environment variables yet.

@facebook-github-bot
Copy link

Hi @pperanich!

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 [email protected]. Thanks!

@jfResearchEng
Copy link
Contributor

jfResearchEng commented Jan 18, 2022

Thank you for your contribution on python version upgrade! Please also help us review the Facebook CLA, CLA is common for all Facebook public repository contributions.

@jfResearchEng
Copy link
Contributor

jfResearchEng commented Jan 18, 2022

As Python Software Foundation has stopped the support for python3.6 on Dec 2021, and python3.8 is the recommended version (as the latest version with security Maintenance status from Python Foundation) for LabGraph, you can alternatively consider supporting python3.8+ in this Pull Request if there is no quick fix to get cibuildwheel tests passed.
Reference: https://www.python.org/downloads/

@pperanich
Copy link
Author

I'll look into that. The python versioning isn't the issue for the Linux cibuilds; I still expect the manylinux PR I submitted to solve those build issues. I can look into the MacOS and Windows builds again to see if supporting 3.8+ simplifies the build in any way.

@pperanich
Copy link
Author

@jfResearchEng could you provide instructions on how the PyPi wheels were built for Windows and Mac? What were the steps to setup the build environment? That could help solve the issues in the cibuildwheel action.

@jfResearchEng
Copy link
Contributor

@jfResearchEng could you provide instructions on how the PyPi wheels were built for Windows and Mac? What were the steps to setup the build environment? That could help solve the issues in the cibuildwheel action.

Instructions on Windows can be found here. Mac is similar with updates for directory in third-party/python and pybinding.

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