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

[ginkgo] Create new port #16536

Merged
merged 4 commits into from
Mar 29, 2021
Merged

[ginkgo] Create new port #16536

merged 4 commits into from
Mar 29, 2021

Conversation

upsj
Copy link
Contributor

@upsj upsj commented Mar 4, 2021

This PR adds a port for the Ginkgo numerical linear algebra library with support for its Reference, OpenMP and CUDA backends. It does have a HIP backend for ROCm GPUs as well, but since the HIP ecosystem can still be a bit fragile, I left it out of the port.

We support and test compilation as a static and dynamic library for Linux (GCC/Clang/Intel), Windows (MSVC, MinGW, Cygwin) and Mac (AppleClang).

As far as I can tell, there is only one other library called Ginkgo, which is a Go testing framework, so name collisions should not be an issue here.

@ghost
Copy link

ghost commented Mar 4, 2021

CLA assistant check
All CLA requirements met.

@upsj upsj force-pushed the master branch 3 times, most recently from 759a05f to 719442b Compare March 4, 2021 17:52
@upsj upsj marked this pull request as draft March 4, 2021 19:07
@PhoebeHui PhoebeHui added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Mar 5, 2021
@PhoebeHui
Copy link
Contributor

@upsj, thanks for your contribution!

The x64-windows failed with following error in CI test.

D:\buildtrees\ginkgo\src\v1.3.0-eec5b92908.clean\core\matrix\dense.cpp : fatal error C1128: number of sections exceeded object file format limit: compile with /bigobj

After you commit all changes, could you please execute the following command to update the baseline version and submmit the changes?

vcpkg x-add-version ginkgo

@upsj
Copy link
Contributor Author

upsj commented Mar 5, 2021

@PhoebeHui Thanks, that's what I guessed as well. There are two possible solutions to this issue: 1. manually add the /bigobj flag to the builds (which is what we do in our CI setup, but I'm not sure that would be considered good practice) or 2. explicitly set symbol visibility, which would be a huge patch (huge being ~1000 line changes), since we don't have a release with this fix out yet. Do you have any policies for this situation?

@PhoebeHui
Copy link
Contributor

PhoebeHui commented Mar 5, 2021

@upsj, you can pass the option via one of the following methods in CMakeLists.txt file.

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /bigobj")

Or

add_compile_options("/bigobj")

@upsj upsj marked this pull request as ready for review March 5, 2021 19:33
@upsj
Copy link
Contributor Author

upsj commented Mar 5, 2021

@PhoebeHui Thanks for your help, I decided to add the flag in our CMakeLists.txt conditional on the compiler. The PR should now be ready to be reviewed.

@PhoebeHui PhoebeHui added the requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function label Mar 8, 2021
@JonLiu1993
Copy link
Member

Hi @upsj ,An error occurred while testing feature: cuda , please take a look at this:
config-x86-windows-out.log
There is also an error when testing feature: openmp:
install-x86-windows-dbg-out.log

@upsj
Copy link
Contributor Author

upsj commented Mar 8, 2021

@JonLiu1993 Thanks for the feedback. I have never tested CUDA with x86 before, did I understand correctly that x64 worked? On the second error, MSVC's OpenMP support is quite outdated, so I guess the right way to approach this would be to disable this feature for windows?

@JonLiu1993
Copy link
Member

@upsj ,I tested the X64 triplet of two features at the same time, but there still seem to be errors,please take a look:
./vcpkg install ginkgo[cuda]:x64-windows
config-x64-windows-out.log
./vcpkg install ginkgo[openmp]:x64-windows
install-x64-windows-dbg-out.log

@upsj
Copy link
Contributor Author

upsj commented Mar 8, 2021

@JonLiu1993 Can you give me some information on the software environment used? It looks like either CUDA isn't installed, or the CUDA MSVC integration doesn't work properly.

@JonLiu1993
Copy link
Member

JonLiu1993 commented Mar 8, 2021

@upsj ,My local environment is Windows 10 with visual studio 2019 and CUDA 11 installed,The cuda version is as follows:
PS F:\test-pr\2\vcpkg> nvcc --version
nvcc: NVIDIA (R) Cuda compiler driver
Copyright (c) 2005-2020 NVIDIA Corporation
Built on Mon_Nov_30_19:15:10_Pacific_Standard_Time_2020
Cuda compilation tools, release 11.2, V11.2.67
Build cuda_11.2.r11.2/compiler.29373293_0

@upsj
Copy link
Contributor Author

upsj commented Mar 8, 2021

@JonLiu1993 That is the same environment I built in, and the configuration seems to work out of the box there. I would suspect that this is a build environment issue with the CUDA Visual Studio integration, see a related SO discussion. Though I am not sure how to proceed with this, since you probably don't want to modify your environment too much just for this specifically. Other packages maybe add specific workarounds for Windows CUDA support, but we just rely on CMake's CUDA support, which requires a correctly installed Visual Studio integration for the same build tools that are used by CMake.

@PhoebeHui
Copy link
Contributor

I also encountered same issue when test the features with x64-windows via vs2019 and cuda 10.2, I tried this in my 3 test machines, all failed with same failures, and I check the 'C:\Program Files\NVIDIA GPU Computing Toolkit\CUDA\v10.2\bin' has been added to the PATH, and the 2 below enviroment variables are correctly set.

image

Failures:

CMake Error at third_party/CMakeLists.txt:2 (enable_language):
  No CMAKE_CUDA_COMPILER could be found.

  Tell CMake where to find the compiler by setting either the environment
  variable "CUDACXX" or the CMake cache entry CMAKE_CUDA_COMPILER to the full
  path to the compiler, or to the compiler name if it is in the PATH.

@upsj, could you help double confirm if the feature 'cuda' test pass locally? if so, it will not block this PR.

For feature 'OpenMP', please disable it.

@upsj
Copy link
Contributor Author

upsj commented Mar 9, 2021

@PhoebeHui There is something really weird going on, since a plain cmake .. build finds a CUDA compiler without issue, but the vcpkg invocation fails. I am investigating this further.

On OpenMP, is it possible to selectively disable a feature just for windows? It looks to me like only dependencies can be made conditional on the platform, but not features?
cl is the only major compiler to not support at least OpenMP 3.0, which has been released in 2008, so I would really like to keep the support for other compilers.

@upsj
Copy link
Contributor Author

upsj commented Mar 9, 2021

@PhoebeHui Following up on my investigation, this is an issue with CMake's Ninja generator and the fact that vcpkg overwrites the Path environment variable, so it can't be fixed in this setting directly. How should we proceed? Merge the project anyways and note that it doesn't support any features on Windows, or wait until this gets fixed in CMake upstream and the default version of CMake gets updated?

@PhoebeHui
Copy link
Contributor

@upsj, thanks for reporting the issue to cmake!

For feature 'OpenMP', since it doesn't work on windows, however, we don't have filed to indicate the supports for a feature, could you add a note in 'description' instead.

It's fine to note that it doesn't support any features on Windows, It should not block this PR, I will approve this PR after the baseline version issue fixed.

BTW, you can also use './vcpkg x-add-version --overwrite-version ginkgo' to fix the baseline version issue.

Error: While reading versions for port ginkgo from file: C:\a\1\s\versions\g-\ginkgo.json
       File declares version `1.3.0` with SHA: 9748b66062d3e27d5310f578953c6aa80adb034f
       But local port with the same verion has a different SHA: 2c686e4d669d1580c9cc84e808376d2189fee613
       Please update the port's version fields and then run:

           vcpkg x-add-version ginkgo

       to add a new version

@PhoebeHui PhoebeHui removed the requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function label Mar 10, 2021
@PhoebeHui PhoebeHui added the info:reviewed Pull Request changes follow basic guidelines label Mar 11, 2021
@ras0219-msft
Copy link
Contributor

ras0219-msft commented Mar 11, 2021

Thanks for the PR! This looks really good, though I'd like to reduce the patches a bit more to get us as close to upstream as possible. No action required on your part.

I've pushed a reduction to the CMake changes at https://github.com/ras0219-msft/vcpkg/dev/roschuma/ginkgo which works on Linux and I've temporarily rolled back the windows-iterator patch so I can see the CI logs (since you've been rebase-pushing, I can't see precisely what the patch is fixing). If it is a warning, we'd prefer to fix it via turning off the warning instead of changing the code.

As far as I can tell, there is only one other library called Ginkgo, which is a Go testing framework, so name collisions should not be an issue here.

There does appear to be https://github.com/OpenMandrivaAssociation/ginkgo/blob/4.0/ginkgo.spec, however it looks like this is an unmaintained GUI program and isn't suitable for packaging in vcpkg. Additionally, it looks like this is packaged in Gentoo as ginkgo as well, so using the name ginkgo here LGTM :)

@upsj
Copy link
Contributor Author

upsj commented Mar 11, 2021

@ras0219-msft Thanks for the update! The windows patch is unfortunately necessary, since our build broke due to a MSVC standard library update and an incomplete implementation of the RandomAccessIterator concept. The CMake changes could probably be minimized though, that is true. Do you squash PRs? If so, I will keep my history in the future.

EDIT: Also, we will probably release a new minor version soon, which should remove the need for any patches, since we've been working heavily on CMake portability recently.

@ras0219-msft
Copy link
Contributor

Yes, we squash PRs. Thanks for the info about Windows.

My reduced branch https://github.com/ras0219-msft/vcpkg/dev/roschuma/ginkgo appears to work -- could you git pull https://github.com/ras0219-msft/vcpkg dev/roschuma/ginkgo into your master to update this PR with those changes?

@PhoebeHui PhoebeHui added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Mar 12, 2021
@upsj
Copy link
Contributor Author

upsj commented Mar 12, 2021

Seems to work for me as well, thanks!

@PhoebeHui PhoebeHui added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Mar 15, 2021
@vicroms vicroms merged commit 286fa50 into microsoft:master Mar 29, 2021
@upsj upsj mentioned this pull request Oct 13, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants