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

Enable DNNL_EXPERIMENTAL_UKERNEL, as needed for PyTorch #76

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

mgorny
Copy link

@mgorny mgorny commented Nov 9, 2024

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

In order to unvendor onednn from PyTorch feedstock (conda-forge/pytorch-cpu-feedstock#289), onednn needs to be built with DNNL_EXPERIMENTAL_UKERNEL enabled.

Please also note that the current version of PyTorch requires onednn 3.5.3, so we are also going to request creating a branch with the equivalent patch added for that version.

In order to unvendor onednn from PyTorch feedstock
(conda-forge/pytorch-cpu-feedstock#289), onednn needs to be built with
DNNL_EXPERIMENTAL_UKERNEL enabled.

Please also note that the current version of PyTorch requires onednn
3.5.3, so we are also going to request creating a branch with
the equivalent patch added for that version.
@conda-forge-admin
Copy link
Contributor

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 (recipe/meta.yaml) and found it was in an excellent condition.

@hmaarrfk
Copy link

hmaarrfk commented Nov 9, 2024

I swear I had written this:

  • Try to enable this feature on windows to keep "feature parity"
  • You might need to conditionally enable it for different target_platforms.

The experimental ukernel code seems to be referencing routines that
are only implemented in `src/cpu/x64`, and it fails to build on aarch64
and ppc64.  Restrict enabling it to x64 appropriately.
@mgorny
Copy link
Author

mgorny commented Nov 11, 2024

Hopefully I've done this right. Somewhat surprised it doesn't compile — because unless I'm missing something, PyTorch enabled it unconditionally for all architectures. But I guess we'll cross that bridge when we get to it.

@mgorny
Copy link
Author

mgorny commented Nov 11, 2024

Hmm, alternatively I think we could enable ukernel on all architectures but disable building examples. Or we can leave it like this for now, and see how that works out for PyTorch.

@vpirogov
Copy link

@mgorny, -DDNNL_EXPERIMENTAL_UKERNEL=ON enables an experimental feature microkernel API, which does not guaranteed API compatibility at this point (that's why it's marked 'experimental'). Enabling this feature by default may cause compatibility issues down the road for users of the binary packages. I see two options to move forward:

  • If you need a conda-forge package with this feature enabled right now I would suggest creating new configuration (or configurations) exactly as needed for Pytorch.
  • If you can wait the feature would become a part of standard distribution once it is promoted from experimental to production status.

@mgorny
Copy link
Author

mgorny commented Nov 12, 2024

Well, we'd rather shave off some build time off PyTorch right now, so I'll make the new configuration. Thanks!

@mgorny
Copy link
Author

mgorny commented Nov 12, 2024

@vpirogov, is this what you had in mind?

recipe/meta.yaml Outdated Show resolved Hide resolved
recipe/meta.yaml Outdated Show resolved Hide resolved
@hmaarrfk
Copy link

I would like to see real improvements in the PyTorch recipe before we merge this.

Could you upload the package you need to your own channel and enable your channel in a PR to PyTorch to demonstrate the gains.

In my experience, the slow compilation was all about CUDA

@mgorny
Copy link
Author

mgorny commented Nov 13, 2024

I would like to see real improvements in the PyTorch recipe before we merge this.

Could you upload the package you need to your own channel and enable your channel in a PR to PyTorch to demonstrate the gains.

Sure, will do that.

In my experience, the slow compilation was all about CUDA

That's entirely possible — to be honest, I haven't gotten to CUDA builds yet, and focused on pure CPU variant so far. According to my local builds, unvendoring onednn shaved ~10% of libtorch build time (as in the, first feedstock part, before specific pytorch packages started being built).

@hmaarrfk
Copy link

Yes. 10% gain on the CPU is what I expect on the CPU front.

It’s just that this is very modest given the unstable API (I was unaware they were using it prior to this PR)

But maybe this 10% gain help get us back on track for the GPU builds in terms of the disk space errors we were seeing. So I think the experiment is worthwhile.

@vpirogov
Copy link

@vpirogov, is this what you had in mind?

Yes, thank you.

@mgorny
Copy link
Author

mgorny commented Nov 13, 2024

@hmaarrfk, done. I've built 3.5.3 with my changes (rebased from this pull request), and updated conda-forge/pytorch-cpu-feedstock#289 to use it. With this, I was able to get pytorch to build and test successfully, though I've used linux_64_blas_implgenericc_compiler_version13c_stdlib_version2.17cuda_compilerNonecuda_compiler_versionNonecxx_compiler_version13 setup for my first attempt.

recipe/meta.yaml Outdated Show resolved Hide resolved
Co-authored-by: Isuru Fernando <isuruf@gmail.com>
Co-authored-by: Isuru Fernando <isuruf@gmail.com>
@mgorny
Copy link
Author

mgorny commented Nov 21, 2024

@hmaarrfk, sorry to bother you, but did you have a chance to test these changes?

recipe/meta.yaml Outdated Show resolved Hide resolved
@conda-forge-admin
Copy link
Contributor

conda-forge-admin commented Jan 3, 2025

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 (recipe/meta.yaml) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe/meta.yaml:

  • ℹ️ The recipe is not parsable by parser conda-souschef (grayskull). This parser is not currently used by conda-forge, but may be in the future. We are collecting information to see which recipes are compatible with grayskull.
  • ℹ️ The recipe is not parsable by parser conda-recipe-manager. The recipe can only be automatically migrated to the new v1 format if it is parseable by conda-recipe-manager.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/12597937481. Examine the logs at this URL for more detail.

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.

5 participants