Skip to content

Update 2.1.0 again - #203

Merged
hmaarrfk merged 13 commits into
conda-forge:mainfrom
hmaarrfk:update_2.1.0_again
Nov 10, 2023
Merged

Update 2.1.0 again#203
hmaarrfk merged 13 commits into
conda-forge:mainfrom
hmaarrfk:update_2.1.0_again

Conversation

@hmaarrfk

@hmaarrfk hmaarrfk commented Nov 7, 2023

Copy link
Copy Markdown
Contributor

@jakirkham the same error happened again.

The CI should show it too:

  -- Found Threads: TRUE
  CMake Error at cmake/public/cuda.cmake:65 (message):
    Found two conflicting CUDA installs:

    V12.0.76 in
    '/home/conda/feedstock_root/build_artifacts/pytorch-recipe_1699320420794/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeh/targets/x86_64-linux/include'
    and

    V12.0.76 in
    '/home/conda/feedstock_root/build_artifacts/pytorch-recipe_1699320420794/_build_env/targets/x86_64-linux/include'
  Call Stack (most recent call first):
    cmake/Dependencies.cmake:44 (include)
    CMakeLists.txt:722 (include)

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.

@conda-forge-webservices

Copy link
Copy Markdown
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) and found it was in an excellent condition.

@jakirkham

Copy link
Copy Markdown
Member

Thanks Mark! 🙏

It appears this is due to a change introduced in PyTorch 2.1.0's own CTK detection logic. Unfortunately this check is problematic

In the Conda case, we implement a splayed layout. This means build tools (like those listed in requirements/build) live in one path and libraries that are linked to (like those listed in requirements/host) live in another path. This is common when supporting cross-compilation as we do with Conda. However this means there are cases where we do need to use some things from both paths for different reasons

It looks like @ax3l ran into this issue in the HPC SDK use case and proposed a fix ( pytorch/pytorch#108932 ). Idk if that will work for us

In terms of our needs here, maybe just patching the check out altogether would be reasonable to get the build working

Perhaps this would be a good opportunity to discuss with @peterbell10 whether we can come up with a better check in PyTorch that works for splayed layout use cases

@hmaarrfk

hmaarrfk commented Nov 7, 2023

Copy link
Copy Markdown
Contributor Author

ok lets try again, i took a brute force approach because we can do a bit of meta building

Comment thread recipe/meta.yaml Outdated
@hmaarrfk

hmaarrfk commented Nov 7, 2023

Copy link
Copy Markdown
Contributor Author

It now fails with:

  CMake Error at cmake/public/cuda.cmake:64 (message):
    Failed to find nvToolsExt
  Call Stack (most recent call first):
    cmake/Dependencies.cmake:44 (include)
    CMakeLists.txt:722 (include)


@jakirkham

Copy link
Copy Markdown
Member

Thanks Mark! 🙏

Think that refers to this check
added in the same PyTorch PR

As noted under the CUDA::nvToolsExt doc in CMake, this is a deprecated target by CMake (and NVIDIA) as it comes from NVTX 2, NVTX 3 has superseded it

Am guessing PyTorch doesn't use NVTX 2 (as this check was new in that PR). Meaning that this was purely a build configuration check

So think we can remove those lines as well

@hmaarrfk

hmaarrfk commented Nov 7, 2023

Copy link
Copy Markdown
Contributor Author

@hmaarrfk

hmaarrfk commented Nov 7, 2023

Copy link
Copy Markdown
Contributor Author

can i remove it too?

@jakirkham jakirkham left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm...think you make a good point

In looking through the code see some references to NVTX 2

We could override where PyTorch looks for NVTX, but maybe it is best to point PyTorch to where the bulk of the CTK content is in the native compilation case

It's possible this will need tweaking further in the build, but hopefully this unblocks configuration

Comment thread recipe/build_pytorch.sh Outdated
@hmaarrfk

hmaarrfk commented Nov 7, 2023

Copy link
Copy Markdown
Contributor Author

ok well i pushed my changes, feel free to push anything if you can. going to slee....

@jakirkham

Copy link
Copy Markdown
Member

Thanks Mark! 🙏

Have a good night

pytorchmergebot pushed a commit to pytorch/pytorch that referenced this pull request Nov 8, 2023
Closes #108931, closes #108932, see also conda-forge/pytorch-cpu-feedstock#203

Currently we compare `CUDA_INCLUDE_DIRS` and expect exact equality
with `CUDAToolkit_INCLUDE_DIR` however this fails in the presense of
symbolic links or for split installs where there are multiple include paths.
Given that, it makes sense to loosen the requirement to just version
equality under the assumption that two installs of the same version
should still be compatible.
Pull Request resolved: #113174
Approved by: https://github.com/malfet
@jakirkham

Copy link
Copy Markdown
Member

It looks like Peter added a fix upstream ( pytorch/pytorch#113174 ). Thanks Peter! 🙏

Maybe we can give that a try

@hmaarrfk

hmaarrfk commented Nov 8, 2023

Copy link
Copy Markdown
Contributor Author

it doesn't address the nvtools issue

@Tobias-Fischer

Copy link
Copy Markdown
Contributor

There are two upsteam PRs that solve the nvtools issue; I am not sure which one is preferable. See pytorch/pytorch#101135 and PRs pytorch/pytorch#97582 and pytorch/pytorch#106763

Comment thread recipe/build_pytorch.sh
@hmaarrfk

hmaarrfk commented Nov 9, 2023

Copy link
Copy Markdown
Contributor Author

ok builds are incoming:

My test was to use

import torch
a = torch.randn(1024 * 1024 * 1024, device='cuda')
a + 1

and watch the memory on my cuda device grow using nvtop

@hmaarrfk
hmaarrfk marked this pull request as ready for review November 9, 2023 14:02
Comment thread recipe/meta.yaml
skip: true # [cuda_compiler_version == "10.2"]
skip: true # [cuda_compiler_version == "11.0"]
skip: true # [cuda_compiler_version == "11.1"]
skip: true # [cuda_compiler_version != "12.0"]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Guessing this is a temporary measure to reign in builds offline. Is that right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes. I can add a note about it. but I'm not thrilled about using 8OSX CPUs for 8-12 hours for this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you mean the CI CPUs? If so, can't we stop the Azure builds manually? Or do you need help with local builds?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't know how to stop the build manually. I just feel that somebody will remember to enable the osx builds.....

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agreed :)

@jakirkham

Copy link
Copy Markdown
Member

Nice work Mark! 🥳

@peterbell10 do the patches here look upstreamable to you? Or are there similar approaches that upstream could take that would alleviate the need for these?

@hmaarrfk

Copy link
Copy Markdown
Contributor Author

log_files.zip

@hmaarrfk
hmaarrfk merged commit 7c7a57b into conda-forge:main Nov 10, 2023
Skylion007 pushed a commit to Skylion007/pytorch that referenced this pull request Nov 14, 2023
Closes pytorch#108931, closes pytorch#108932, see also conda-forge/pytorch-cpu-feedstock#203

Currently we compare `CUDA_INCLUDE_DIRS` and expect exact equality
with `CUDAToolkit_INCLUDE_DIR` however this fails in the presense of
symbolic links or for split installs where there are multiple include paths.
Given that, it makes sense to loosen the requirement to just version
equality under the assumption that two installs of the same version
should still be compatible.
Pull Request resolved: pytorch#113174
Approved by: https://github.com/malfet
@jakirkham

jakirkham commented Nov 29, 2023

Copy link
Copy Markdown
Member

Were Linux ARM packages built as well?

Edit: Nvm I see them 🤦‍♂️

Screenshot 2023-11-29 at 12 29 01 PM

@jakirkham jakirkham mentioned this pull request Mar 8, 2024
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