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

Add support for external installations of key dependencies #24

Merged
merged 29 commits into from
Aug 6, 2024

Conversation

sethrj
Copy link
Member

@sethrj sethrj commented Jul 19, 2024

Allowing external dependencies rather than "vendoring" them (by downloading and building inline) is a best practice for scientific software stacks. It prevents errors if a downstream client wants to also link to those dependencies (or if they use another app that links against them), and it also reduces the overhead in a edit-compile-debug cycle, especially on CI systems.

This is the first step in that direction. It allows several packages to work with already built externals:

  • Boost
  • cpr
  • GTest
  • spdlog
  • nlohmann_json@3.1
  • Eigen3

The following spack environment lets XACC use the externals:

spack:
  specs:
  - cpr@1.9
  - boost+graph
  - nlohmann-json@3.1
  - eigen@3
  - git
  - lapack
  - spdlog
  - python
  - googletest
  view: true
  concretizer:
    unify: true

cc @wongey

UPDATE: the one problem I'm seeing is that something very screwy is going on with the xacc dependencies. The first configure, everything builds find. After reconfiguring, the eigen include directory simply disappears from the build.ninja file line. However the default "unix makefiles" option seems to work fine, so this may be related to ninja-build/ninja#1251 or something else weird...

@sethrj sethrj requested a review from danclaudino July 19, 2024 12:56
@wongey
Copy link

wongey commented Jul 19, 2024

Build is good for this PR on my end (but was not necessarily faster and still takes time to download the files during cmake). Also works well with qiree.

@sethrj
Copy link
Member Author

sethrj commented Jul 19, 2024

@wongey Cool. If you use spack (or if you use dnf or some other utility) to install the prereqs separately and enable XACC_DEPS_EXTERNAL, it'll reduce your build time substantially.

@danclaudino
Copy link
Member

@sethrj Could you share how you handle the dependencies through the spack environment and whether building XACC with requires any changes other than switching XACC_DEPS_EXTERNAL on? I'm not familiar with spack and want to make sure I'm doing the right thing. It takes a very long time to sort out the dependencies, building doesn't seem any faster, and I'm getting various cmake warnings.

@sethrj
Copy link
Member Author

sethrj commented Jul 22, 2024

@danclaudino 👍

  1. Create a spack environment using the yaml, activate it, and spack install. I don't think it should take that long but there is some extra overhead the first time you run spack.
  2. With the environment activated, the installations will be added to your cmake prefix path, and you can just install xacc.

My build script is:

cmake \
  -DQIREE_MINIMAL_BUILD=ON \
  -DXACC_DEPS_EXTERNAL=ON \
  -DXACC_BUILD_TESTS=OFF \
  -DXACC_BUILD_EXAMPLES=OFF \
  ..

The main benefit for our use case of installing dependencies externally is that if you make a new build of xacc you don't have to redownload and reinstall these every time. This is particularly potent for CI builds which is the primary driver. The second benefit is that it means you can play nicely with other software frameworks: if QIREE wants to link another software project built with JSON, we can do so without nasty ODR violations. One final benefit is that you can build XACC as debug but still use optimized versions of the upstream utilities.

@wongey
Copy link

wongey commented Jul 23, 2024

@sethrj @danclaudino I used the above spack environment together with your build script. The environment setup was unfortunately quite slow (~30 minutes). It was also sidetracked by an incompatible gcc version (which added additional minutes on my end to figure out). In the end, I would say using the environment overall did not improve the original cmake time significantly (at least for me).

Also, I had to add add_compile_definitions(SPDLOG_FMT_EXTERNAL) to the top level CMakeList for it to compile properly.

In any case, both minimal builds (with or without spack) were successful and works with qiree.

@sethrj
Copy link
Member Author

sethrj commented Jul 23, 2024

Ah, I forgot to mention one key step that shortens build time: running spack external find before installing lets spack use existing packages on your system such as cmake and python, rather than building from scratch. And remember, the external environment installation is a one-time cost.

Here, all the e packages are external. Python could be as well.
Screenshot 2024-07-23 at 07 18 56

@wongey If you had to add that flag then something's not right...

@wongey
Copy link

wongey commented Jul 30, 2024

With @sethrj's new commits in this PR, the xacc build in the spack environment is smooth for me with gcc@11.4.0. I did not have to make further modifications. Normal cmake is fine is as well.

@sethrj
Copy link
Member Author

sethrj commented Jul 30, 2024

Thanks for the verification @wongey !

@sethrj
Copy link
Member Author

sethrj commented Jul 31, 2024

@danclaudino Would you mind merging this as soon as possible? It seems like we're stepping on each others' change sets which results in a fair bit of extra work for conflict resolution. I'll pull in the upstream changes and perhaps @wongey could give it a final once over if you're ready to merge? Thanks!

@sethrj
Copy link
Member Author

sethrj commented Jul 31, 2024

And also the merge/diff process is significantly complicated by the re-indentation of parameters in CMake files.
For example:

add_library(xacc SHARED
            xacc.cpp
            accelerator/AcceleratorBuffer.cpp
            utils/Utils.cpp
            utils/CLIParser.cpp
            utils/heterogeneous.cpp
            ir/IRBuilder.cpp
            service/ServiceRegistry.cpp
            service/xacc_service.cpp
            accelerator/remote/RemoteAccelerator.cpp)

to

file(GLOB SOURCES
    xacc.cpp
    accelerator/AcceleratorBuffer.cpp
    utils/Utils.cpp
    utils/CLIParser.cpp
    utils/heterogeneous.cpp
    ir/IRBuilder.cpp
    service/ServiceRegistry.cpp
    service/xacc_service.cpp)

means changing every single line, whereas

file(GLOB HEADERS
  xacc.hpp
  ir/*.hpp
  compiler/Compiler.hpp
  accelerator/*.hpp
  utils/*.hpp
  service/*.hpp
  algorithm/*.hpp
  optimizer/*.hpp
)

means that changes will only affect a single line and new list elements can be inserted at the end.

@danclaudino danclaudino merged commit 566d63a into ORNL-QCI:master Aug 6, 2024
15 of 16 checks passed
@sethrj sethrj deleted the external branch August 6, 2024 15:19
@sethrj
Copy link
Member Author

sethrj commented Aug 6, 2024

Thanks @danclaudino !

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.

3 participants