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.

Sign up for GitHub

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

Jump to bottom

Add Manylinux Python 3.7 wheel build + tests, disable Azure Windows Python build #1010

Merged
merged 18 commits into from
Apr 14, 2020
Merged

Add Manylinux Python 3.7 wheel build + tests, disable Azure Windows Python build #1010

merged 18 commits into from
Apr 14, 2020

Conversation

sc1f
Copy link
Contributor

@sc1f sc1f commented Apr 10, 2020

This PR adds wheel builds and install tests for Perspective's manylinux2010 and manylinux2014 images on Python 3.7, storing built artifacts on Azure's pipeline artifact storage.

Additionally, this PR temporarily disables the Windows Python 3.7 build as it seems that Boost's version/path has changed in more recent releases of the Windows VM on Azure Pipelines, which breaks all builds and requires further investigation.

Changes

After builds/tests on manylinux are completed, a wheel is built and installed in a fresh virtual environment. perspective is then imported and is_libpsp called to check that paths to all built C++ shared libraries resolve properly as part of the wheel install. This check is performed for every branch push to CI.

I added a tags trigger to our pipeline, which means that a wheel will be built on each new tag, as well as when the commit containing the new tag is merged into master. There is no support for automatic disting/file upload (using jpmorganchase/nbcelltests as a guide to that end), but that can be built in as part of a more comprehensive release process.

Caveats

  • The Python 2.7 manylinux2010 wheel is shown as incompatible when trying to install in a fresh virtual environment; I assume this is due to some technicality of wheels/virtualenv in Python 2 but am not investigating further for the time being
  • Ideally, this task should be enabled for MacOS 10.14 and MacOS 10.15 as well, which would allow us to build Mac wheels quickly as part of the release process.

@sc1f sc1f added the internal Internal refactoring and code quality improvement label Apr 10, 2020
@sc1f sc1f requested a review from timkpaine April 10, 2020 22:27
@timkpaine
Copy link
Member

actions/runner-images#319

@timkpaine
Copy link
Member

try setting Boost_ROOT to BOOST_ROOT_1_72_0

@sc1f sc1f force-pushed the azure-wheel branch 2 times, most recently from 5210e26 to 00fc48b Compare April 11, 2020 01:13
@timkpaine
Copy link
Member

try not to pollute the CMakeLists.txt, I think you can do it in the pipeline definition like you were trying to do

@sc1f sc1f force-pushed the azure-wheel branch 3 times, most recently from d5309bd to cbb22c0 Compare April 11, 2020 01:40
@sc1f
Copy link
Contributor Author

sc1f commented Apr 11, 2020

Screen Shot 2020-04-10 at 9 08 41 PM

Our version of CMake seems to ignore the env var even if it's set - I can try setting the include dir and library dir and somehow forcing CMake to read the env var.

Looking at some of the linked PRs/commits from the issue on virtual-environments:

https://github.com/giotto-ai/giotto-tda/pull/388/files

This might be a solution, just pulling in Boost and installing it in a directory we know to be correct.

@timkpaine
Copy link
Member

@sc1f as a last test can you try adding -DBoost_ROOT=%BOOST_ROOT_1_69_0% to the cmake command

@sc1f
Copy link
Contributor Author

sc1f commented Apr 11, 2020

It seems like neither BOOST_ROOT_1_69_0 or BOOST_ROOT_1_72_0 are defined, unless my check for whether it's defined is incorrect

@sc1f
Copy link
Contributor Author

sc1f commented Apr 11, 2020 •
edited
Loading

Saving this here for posterity:

if (WIN32)
  # BOOST_ROOT has been removed on Windows VMs in Azure - 
  # add BOOST_ROOT_1_72_0 so it can find the custom installation of
  # boost on those VMs:
  # https://github.com/actions/virtual-environments/issues/6870
  # FIXME: do not let this get merged
  message("SETTING BOOST TO C:/hostedtoolcache/windows/Boost/1.72.0/")
  SET (BOOST_ROOT "C:/hostedtoolcache/windows/Boost/1.72.0/")
  SET (BOOST_INCLUDEDIR "C:/hostedtoolcache/windows/Boost/1.72.0/include")
  SET (BOOST_LIBRARYDIR "$C:/hostedtoolcache/windows/Boost/1.72.0/lib")
  SET (Boost_NO_BOOST_CMAKE ON)
  message("BOOST_INCLUDEDIR: ${BOOST_INCLUDEDIR}")
  message("BOOST_LIBRARYDIR: ${BOOST_LIBRARYDIR}")
endif()

This passed the build, so I'm pushing up a less janky reformulation of it.

@timkpaine
Copy link
Member

Cool. You can pass these args into the cmake command with just -DBOOST_ROOT=.... I would say if we're doing yarn build_python --ci and we detect we're on windows, add those flags (to keep the CMakeLists.txt clean)

@sc1f
Copy link
Contributor Author

sc1f commented Apr 11, 2020

Looks like what I pushed up fixes it (or has at least gotten past the point where it usually breaks) - no more CMake crowding except for setting policy CMP0074 explicitly to NEW, and then just reading BOOST_ROOT from the pipelines YAML.

timkpaine
timkpaine reviewed Apr 11, 2020
View reviewed changes
cpp/perspective/CMakeLists.txt Outdated
if (WIN32)
# BOOST_ROOT has been removed on Windows VMs in Azure:
#
# - https://github.com/actions/virtual-environments/issues/6870
Copy link
Member

Choose a reason for hiding this comment

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

404

@timkpaine
Copy link
Member

cool this should make deploying the wheels much quicker and easier. Maybe @texodus can merge this and a few of the other ones that are ready and cut a 0.4.8 to test the full process

@sc1f
Copy link
Contributor Author

sc1f commented Apr 11, 2020

Don't merge yet lol

https://dev.azure.com/finosfoundation/perspective/_build/results?buildId=434&view=logs&j=bfe8a88d-3928-550a-8c7c-c7cdeca2abfc&t=5e12ee82-6ac3-5dc7-8a82-27ab9e7f635f&l=579

Looks like a linker error - consistent in both PR and commit builds...

@sc1f
Copy link
Contributor Author

sc1f commented Apr 11, 2020 

Creating library D:/a/1/s/python/perspective/build/temp.win-amd64-3.7/Release/Release/psp.lib and object D:/a/1/s/python/perspective/build/temp.win-amd64-3.7/Release/Release/psp.exp
    utils.obj : error LNK2019: unresolved external symbol BCryptCloseAlgorithmProvider referenced in function "public: __cdecl boost::uuids::random_generator_pure::~random_generator_pure(void)" (??1random_generator_pure@uuids@boost@@QEAA@XZ) [D:\a\1\s\python\perspective\build\temp.win-amd64-3.7\Release\psp.vcxproj]
    utils.obj : error LNK2019: unresolved external symbol BCryptGenRandom referenced in function "class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl perspective::unique_path(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &)" (?unique_path@perspective@@YA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@AEBV23@@Z) [D:\a\1\s\python\perspective\build\temp.win-amd64-3.7\Release\psp.vcxproj]
    utils.obj : error LNK2019: unresolved external symbol BCryptOpenAlgorithmProvider referenced in function "class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl perspective::unique_path(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &)" (?unique_path@perspective@@YA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@AEBV23@@Z) [D:\a\1\s\python\perspective\build\temp.win-amd64-3.7\Release\psp.vcxproj]
    D:\a\1\s\python\perspective\build\temp.win-amd64-3.7\Release\Release\libpsp.dll : fatal error LNK1120: 3 unresolved externals [D:\a\1\s\python\perspective\build\temp.win-amd64-3.7\Release\psp.vcxproj]

@sc1f
Copy link
Contributor Author

sc1f commented Apr 11, 2020

So adding BOOST_ROOT to the path in CMakeLists?

@sc1f sc1f force-pushed the azure-wheel branch 3 times, most recently from 6400661 to 7518714 Compare April 13, 2020 14:20
@sc1f
Copy link
Contributor Author

sc1f commented Apr 13, 2020

Looks like downgrading Boost to 1.69.0 (from 1.72.0) worked. AFAIK the version of boost that we need is anything above 1.67.0, which means this should be fine. Re-running builds now, but I saw the last build pass for Python (with a flapping test), so all should be good.

@texodus
Copy link
Member

texodus commented Apr 14, 2020

Thanks for the PR! Looks good!

@texodus texodus merged commit 319723d into master Apr 14, 2020
@texodus texodus deleted the azure-wheel branch April 14, 2020 04:18
@timkpaine timkpaine mentioned this pull request Apr 16, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timkpaine timkpaine timkpaine approved these changes

Assignees
No one assigned
Labels
internal Internal refactoring and code quality improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sc1f @timkpaine @texodus @finos-admin