OIDN 1.2.0 + mkl-dnn 1.2.2 fails to build from source

[jasonliu--]

Hello, I'm currently working on packaging OIDN for the MacPorts package manager. In the course of creating my package, I've found that OIDN fails to compile from source if I download the 1.2.2 release of OIDN's fork of mkl-dnn. However, if I clone mkl-dnn from master (specifically https://github.com/OpenImageDenoise/mkl-dnn/commit/8158955c358821dcd08b63c65f0b0b75895b244e), then OIDN compiles successfully.

The MacPorts build system has a convenient way to download tagged releases from Github, but it's more of a hassle if I have to actually clone the master branch. Is there any way that you might be able to tag a new release of mkl-dnn, so that downloading a combination of tagged releases (e.g. oidn-1.2.0.tar.gz + mkl-dnn-1.2.3.tar.gz + oidn-weights-1.2.tar.gz) will compile successfully from source?

[atafra]

Hi! Thanks for reaching out! OIDN does not compile with tagged releases of mkl-dnn, it works only with the forked version. You shouldn't manually clone mkl-dnn, and actually you don't have to. The oidn repo has mkl-dnn as a submodule, and it references the correct mkl-dnn commit. So the solution is to always clone OIDN with all submodules recursively.

[jasonliu--]

The mkl-dnn submodule is precisely what I am talking about. If you click on the commit that I reference in my original post, you will see that it links to the mkl-dnn fork that is a submodule of OIDN. (Sorry if I caused confused when I said "master". I was talking about the master of the mkl-dnn submodule fork, and not the original mkl-dnn/DNNL/oneDNN.) The releases in the mkl-dnn submodule is currently at 1.2.2, and I am asking whether you would be willing to tag commit OpenImageDenoise/mkl-dnn@8158955 as a new release.

Like I said, it's actually quite a big hassle to do a git clone from inside the MacPorts build system. It prefers to download tagged releases, and if you download a tagged release, you can't do a git clone once you untar it, because a tagged release doesn't count as a valid github repo. The MacPorts developers very strongly prefer package submissions to be based on tagged releases, and not an individual commit from the master branch... any submissions like that are likely to be rejected.

[atafra]

I still don't understand why do you need to clone mkl-dnn separately from oidn. The whole point of submodules is to clone the main repository (oidn) with the --recursive option and it will clone all referenced submodules with the right commits as well. The main repository, oidn, is tagged and that is all you need to download and build. If doing a git clone is too difficult, you can just download the whole oidn source package which contains all submodules as well.

The master branch of the mkl-dnn repository is actually the same as the official one, it's just not up to the date. The forked code that oidn uses is in the oidn branch. But the modified code is not supposed to be used independently from oidn (it might not even compile), so we won't tag it as a new release because it is not. The fork is linked statically to oidn because of this, so why does MacPorts care whether all submodules are tagged as releases or not? The mkl-dnn fork that oidn uses is not an independent library. The only library that needs to be packaged is oidn itself, which has tagged releases.

What I also don't understand is why the mkl-dnn submodule is the only issue, because it's not the only submodule that oidn uses. What about the oidn-weights submodule (which is not even a library, it's just data)?

Thanks!

[jasonliu--]

I still don't understand why do you need to clone mkl-dnn separately from oidn.

I'm not cloning mkl-dnn separately from oidn. As I have said a few times already, the MacPorts build system makes performing a git clone a hassle, which means that I am not cloning anything. However, the MacPorts build system is able to download .tar.gz files just fine (probably using something like wget or curl), so the MacPorts build system makes it very easy to fetch a tagged release.

you can just download the whole oidn source package which contains all submodules as well.

No, the oidn source package does not contain the submodules. If you download the oidn source package from here: https://github.com/OpenImageDenoise/oidn/archive/v1.2.0.tar.gz and extract the archive, you will see that both the mkl-dnn and weights folders are empty.

But the modified code is not supposed to be used independently from oidn (it might not even compile), so we won't tag it as a new release because it is not.

I agree with you that the modified mkl-dnn code is not supposed to by used independently from oidn. But that doesn't mean that the submodules aren't considered their own separate projects from GitHub.com's perspective, and have their own individual set of tagged releases. Otherwise, how is it possible that these pages exist?

• https://github.com/OpenImageDenoise/mkl-dnn/releases
• https://github.com/OpenImageDenoise/oidn-weights/releases

why does MacPorts care whether all submodules are tagged as releases or not?

Because MacPorts is downloading the .tar.gz files from the two pages I linked to above. Only commits that have been tagged as release will appear on those pages.

What I also don't understand is why the mkl-dnn submodule is the only issue, because it's not the only submodule that oidn uses. What about the oidn-weights submodule (which is not even a library, it's just data)?

I don't understand either, since I didn't write oidn, you did. :) All I can say is that the oidn-weights submodule seems to work fine. Downloading the .tar.gz file from https://github.com/OpenImageDenoise/oidn-weights/releases/tag/v1.2 and extracting it into the weights folder inside of the oidn source code, there were never any errors while compiling. However, when I did the same thing for mkl-dnn by downloading the .tar.gz from https://github.com/OpenImageDenoise/mkl-dnn/releases/tag/v1.2.2 and extracting it into the mkl-dnn folder inside the oidn source code, I was getting build errors (specifically, linking errors near the very end).

How I figured out that the newest commit that you have in the master branch of the mkl-dnn submodule allows oidn to compile correctly (as opposed to the mkl-dnn submodule that has been tagged as release v1.2.2), was by using the following steps. I had to perform all of these steps outside of the MacPorts build system, directly from the command line. You can try them yourself, to see exactly what I am talking about.

(1) cd /tmp
(2) curl -L https://github.com/OpenImageDenoise/oidn/archive/v1.2.0.tar.gz -o oidn-1.2.0.tar.gz
(3) tar xf oidn-1.2.0.tar.gz
(4) cd oidn-1.2.0
(5) cd weights
#   ^^^^ See what I was talking about? weights folder is empty!
(6) curl -L https://github.com/OpenImageDenoise/oidn-weights/archive/v1.2.tar.gz -o oidn-weights-1.2.tar.gz
(7) tar xf oidn-weights-1.2.tar.gz
(8) mv oidn-weights-1.2/* .
(9a) cd ../mkl-dnn
#    ^^^^ Also empty :)
(10a) curl -L https://github.com/OpenImageDenoise/mkl-dnn/archive/v1.2.2.tar.gz -o oidn-mkl-dnn-1.2.2.tar.gz
(11a) tar xf oidn-mkl-dnn-1.2.2.tar.gz
(12a) mv mkl-dnn-1.2.2/* .
(13) cd ..
(14) mkdir build
(15) cd build
# I modified the following command from the normal
# instructions because I am using MacPorts Clang to compile
(16) cmake -DCMAKE_CXX_COMPILER=/opt/local/bin/clang++ -DCMAKE_C_COMPILER=/opt/local/bin/clang ..
# Inside ccmake, I changed the CMAKE_INSTALL_PREFIX to
# somewhere in /tmp, like '/tmp/foobar'
(17) ccmake ..
(18) make

If you use the (a) steps listed above, the build will fail. However, if you use the (b) steps listed below instead, the build will succeed.

(9b) cd ..
(10b) git clone https://github.com/OpenImageDenoise/mkl-dnn.git
# skip directly to Step (13)

As you can see in the (b) steps, I am cloning the mkl-dnn submodule from the oidn fork. Also, remember that I can't do any of this from inside MacPorts... I did this manually to try to troubleshoot why the build was failing.

[jeffamstutz]

To rule out (or not) an idea I have: can the MacPorts build system easily invoke a CMake superbuild, where the superbuild fetches/builds private dependencies? We can have superbuilds point to exact git commit hashes, which futher decouples it from the existence of tags/branches.

We use superbuilds for OpenVKL/OSPRay, just curious if you think it could be applicable here too?

[atafra]

The reason why the release pages for mkl-dnn exist is that it's simply a fork of the original mkl-dnn repository. That doesn't mean that any of those releases should be used for building Open Image Denoise. Just because Open Image Denoise somehow compiles without errors with one of the tagged mkl-dnn releases does not mean that it will work correctly. Most likely it won't. So I strongly advise against doing this.

I'm fully aware that source tarballs automatically generated by GitHub doesn't contain the submodules. Unfortunately it's a limitation of GitHub, and AFAIK there's no way to fix this. However, we're distributing our own full source tarballs for each release on GitHub, and that is the source tarball which we link everywhere (e.g. the website). For example, this is the full source tarball for the latest Open Image Denoise version: https://github.com/OpenImageDenoise/oidn/releases/download/v1.2.0/oidn-1.2.0.src.tar.gz

If it's possible to download a tarball with wget/curl/etc. inside MacPorts, why not download that file instead? That one contains all submodules.

[atafra]

I'm not familiar with the internals of MacPorts but after having a quick look at the official guide (https://guide.macports.org/) I see two possible solutions:

1. Download our custom tarball with all submodules. This seems to be possible by manually specifying distfiles (oidn-{version}.src.tar.gz) instead of using the default one.

2. Use git and fetch all submodules too. The guide has a section which explains how to do this: https://guide.macports.org/#reference.portgroup.github.submodule

Could any of these work? In any case, I think manually fetching the submodules is very error prone, difficult to maintain, and it could easily break the library in some non-obvious way.

[jasonliu--]

I'm fully aware that source tarballs automatically generated by GitHub doesn't contain the submodules. Unfortunately it's a limitation of GitHub, and AFAIK there's no way to fix this. However, we're distributing our own full source tarballs for each release on GitHub, and that is the source tarball which we link everywhere (e.g. the website). For example, this is the full source tarball for the latest Open Image Denoise version: https://github.com/OpenImageDenoise/oidn/releases/download/v1.2.0/oidn-1.2.0.src.tar.gz

If it's possible to download a tarball with wget/curl/etc. inside MacPorts, why not download that file instead? That one contains all submodules.

This one works! I hadn't even noticed that particular tarball in the list of files in the tagged release, since the default file that MacPorts automagically finds and downloads corresponds to the file labeled Source code (tar.gz). Telling MacPorts to get this file instead (yes, using distfiles), everything builds fine without any errors.

And since this file is considered to be a part of the tagged release, I don't have to do anything special like attempting to use git to fetch all of the submodules (which as @atafra points out is indeed very error prone and difficult to maintain, and is generally discouraged by the MacPorts devs).

So in the end, it looks like nothing needed to be changed on your guys' end. As long as you keep distributing a full source tarball in the tagged releases, or GitHub changes things and starts generating tarballs that include the submodules, then this is the simplest and best solution. Thanks so much for all the help, and your responsiveness!

[atafra]

Yes, we're fully committed to distribute these tarballs in the future. I'm glad we could find a nice solution! Thanks for working on this!