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 conda-forge recipie for dgl #22691

Merged
merged 105 commits into from
Aug 9, 2023
Merged

Add conda-forge recipie for dgl #22691

merged 105 commits into from
Aug 9, 2023

Conversation

hmacdope
Copy link
Contributor

@hmacdope hmacdope commented Apr 25, 2023

Tagging @mikemhenry and @hadim

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

Fixes #12537

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/dgl) and found it was in an excellent condition.

@mikemhenry mikemhenry mentioned this pull request Apr 25, 2023
11 tasks
@mikemhenry
Copy link
Contributor

-- DLPACK_INCLUDE_DIR not defined, searching for dlpack header
CMake Error at /home/conda/staged-recipes/build_artifacts/dgl_1682466386125/_build_env/share/cmake-3.26/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find DLPACK (missing: DLPACK_INCLUDE_DIR)
Call Stack (most recent call first):
  /home/conda/staged-recipes/build_artifacts/dgl_1682466386125/_build_env/share/cmake-3.26/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)
  cmake/modules/FindDLPACK.cmake:39 (find_package_handle_standard_args)
  CMakeLists.txt:279 (find_package)

@hmacdope
Copy link
Contributor Author

@mikemhenry this is the same issue I was seeing locally, I will continue to investigate. 😺

@bartekkuncer
Copy link

bartekkuncer commented May 24, 2023

Hi @hmacdope, @mikemhenry! First of all thanks for driving the initiative of adding dgl to conda-forge channel. I have two questions:

  1. how are the works going - is there any major blocker atm?
  2. I noticed that in build.sh you are setting USE_LIBXSMM flag to 'OFF' even though the library is available on conda-forge channel and the flag by default is set to 'ON'. Why is that?

@mikemhenry
Copy link
Contributor

👋 Hey @bartekkuncer Check out this thread here: dmlc/dgl#1855 (you may have seen it) @hmacdope is in Australia so there is a bit of a time delay. We are still in the middle of working on this and will post an update soon. Right now I think that flag controls if to build with libxsmm, and doesn't control using the vendored lib or the lib from conda-forge. Before we merge in this feedstock, we will have it using every lib that is on codna-forge and we will add the missing ones as well.

@hmacdope
Copy link
Contributor Author

@mikemhenry this will compile now at least (import failing with some linker error) which means the mechanism for inserting CF deps into the build is working. I think I will now try and make this work with the latest version + vendored thrust + up to date deps.

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/dgl) and found some lint.

Here's what I've got...

For recipes/dgl:

  • requirements: build: parallel-hashmap == 1.32 should not contain a space between relational operator and the version, i.e. parallel-hashmap ==1.32

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/dgl) and found it was in an excellent condition.

@mikemhenry
Copy link
Contributor

@jakirkham no worries! I saw that it was missing from the docs you linked but I wasn't sure if it was an intentional omission.

@mikemhenry
Copy link
Contributor

I am now trying locally a build where I don't have anything added to the test env to see if adding liblapacke is enough

@mikemhenry
Copy link
Contributor

Without - liblapack =*=*mkl in test we still get libdgl.so: undefined symbol: dgemm_

So I know that having - liblapack =*=*mkl in test gets CI to pass, but I am worried about users running into this issue.

I uploaded the built package and then installed it, and get libdgl.so: undefined symbol: dgemm_ locally.

Then if I install liblapack =*=*mkl

   - liblapack=*[build=*mkl]


  Package            Version  Build                        Channel           Size
───────────────────────────────────────────────────────────────────────────────────
  Install:
───────────────────────────────────────────────────────────────────────────────────

  + libhwloc           2.9.2  nocuda_h7313eea_1008         conda-forge        3MB
  + libiconv            1.17  h166bdaf_0                   conda-forge     Cached
  + libxml2           2.11.4  h0d562d8_0                   conda-forge     Cached
  + llvm-openmp       16.0.6  h4dfa4b3_0                   conda-forge     Cached
  + mkl             2022.2.1  h84fe81f_16997               conda-forge     Cached
  + rocm-smi           5.6.0  h59595ed_1                   conda-forge        4MB
  + tbb            2021.10.0  h00ab1b0_0                   conda-forge      186kB

  Remove:
───────────────────────────────────────────────────────────────────────────────────

  - nomkl                1.0  h5ca1d4c_0                   conda-forge     Cached

  Change:
───────────────────────────────────────────────────────────────────────────────────

  - _openmp_mutex        4.5  2_gnu                        conda-forge     Cached
  + _openmp_mutex        4.5  2_kmp_llvm                   conda-forge     Cached
  - libblas            3.9.0  17_linux64_openblas          conda-forge     Cached
  + libblas            3.9.0  16_linux64_mkl               conda-forge     Cached
  - libcblas           3.9.0  17_linux64_openblas          conda-forge     Cached
  + libcblas           3.9.0  16_linux64_mkl               conda-forge     Cached
  - liblapack          3.9.0  17_linux64_openblas          conda-forge     Cached
  + liblapack          3.9.0  16_linux64_mkl               conda-forge     Cached
  - liblapacke         3.9.0  17_linux64_openblas          conda-forge     Cached
  + liblapacke         3.9.0  16_linux64_mkl               conda-forge     Cached
  - pytorch            2.0.0  cpu_generic_py310h7ffd2bf_1  conda-forge     Cached
  + pytorch            2.0.0  cpu_mkl_py310h402c8e3_101    conda-forge       71MB

The error goes away.

@mikemhenry
Copy link
Contributor

So I don't know why, but it looks like we are dependent on the mkl build of blas, despite not having that in our host env explicitly.

@jakirkham
Copy link
Member

Am confused by the _. The BLAS symbol name is dgemm (no _). Do we know where it is coming up with that?

@mikemhenry
Copy link
Contributor

If you want to play around with this, this command will install dgl (the build that came from CI)
micromamba create -n dlg-cpi -c conda-forge -c "mmh/label/cpu-test" dgl

and python -c "import dgl" should give you an error unless you first do micromamba install "liblapack =*=*mkl"

@mikemhenry
Copy link
Contributor

It looks like they all come from third_party/libxsmm and there is something weird https://github.com/libxsmm/libxsmm/blob/ef75fe7c814c1a8a51f8c142db71059caab65e02/samples/utilities/wrap/dgemm.c#L19

@hmacdope
Copy link
Contributor Author

hmacdope commented Aug 6, 2023

Can we just use MKL for now and then unwind the issues in a feedstock @mikemhenry @jakirkham?

@mikemhenry
Copy link
Contributor

I am going to try moving liblapack =*=*mkl to run: and see if that works (locally) and if it does, push it to CI and we can figure out a better way after we get this merged

@mikemhenry
Copy link
Contributor

Okay so once CI passes, I will remove the stuff I did for the swapfile, that might break the build but then it should be good to go...

@mikemhenry
Copy link
Contributor

@conda-forge-admin, please ping team

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-webservice.

I was asked to ping @conda-forge/staged-recipes and so here I am doing that.

@mikemhenry
Copy link
Contributor

Okay so this might fail without the swap file stuff, but this is ready to merge 😄 @carterbox

- metis ==5.1.1
- nanoflann
- parallel-hashmap
- python =3.10
Copy link
Member

Choose a reason for hiding this comment

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

Was this just due to CI limits?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! Yes! I will fix that now

@mikemhenry
Copy link
Contributor

@jakirkham this is ready to merge, we won't get a green check without the swap fix

@jakirkham
Copy link
Member

Thanks Mike! 🙏

LGTM

@carterbox did you have any more thoughts here?

Also Mike it might be worth sending a separate PR to add the swapfile changes to staged-recipes if you are up for it 🙂

@mikemhenry
Copy link
Contributor

Also Mike it might be worth sending a separate PR to add the swapfile changes to staged-recipes if you are up for it

Will do!

@carterbox carterbox merged commit 68adcb3 into conda-forge:main Aug 9, 2023
@jakirkham
Copy link
Member

Thanks all! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Package dgl
7 participants