deps cleanup #162

ngam · 2023-02-03T01:58:23Z

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 · 2023-02-03T01:58:27Z

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.

ngam · 2023-02-03T01:58:31Z

@conda-forge-admin, please rerender.

…nda-forge-pinning 2023.02.02.19.14.40

recipe/meta.yaml

hmaarrfk · 2023-02-03T03:30:32Z

How did you get around the issues with OSX SDK?

ngam · 2023-02-03T04:33:44Z

I think in the other PR we were sometimes flying somewhat blindly with not rerendering correctly, but also just to be safe I bumped the SDK to the highest. The GitHub team made Xcode 14.2 the default actions/runner-images#6746 which in their docs correspond to SDK 13.1 https://github.com/actions/runner-images/blob/main/images/macos/macos-12-Readme.md#installed-sdks, but I really don't understand why we cannot find the other available SDKs. One reason is the method we use to detect them: It goes through Xcode thus perhaps limiting us to what is associated with the default Xcode https://github.com/conda-forge/conda-forge-ci-setup-feedstock/blob/80f0566fcd3c58ad33331c9d0f0084276c158f30/recipe/run_conda_forge_build_setup_osx#L32.

So anyway, to be safest, I thought we could just target the latest SDK. Fwiw, I think PyTorch no longer passes for anything before 12.3 anyway. I am not sure if there are any real downsides with setting the SDKs higher (not the deployment target) and I don't know why we have it really low by default. Here's the relevant snippet: https://github.com/conda-forge/conda-forge-ci-setup-feedstock/blob/bf1f98d6694b63e9487fbc9817d2bf2d9523b73a/recipe/download_osx_sdk.sh#L7

h-vetinari · 2023-02-03T04:39:02Z

I am not sure if there are any real downsides with setting the SDKs higher (not the deployment target) and I don't know why we have it really low by default.

As always: compatibility for old systems, though of course part of it is also inertia in changing something at all. With _LIBCPP_DISABLE_AVAILABILITY & per-feedstock opt-out from the baseline 10.9, most things can be solved as necessary. And for anything beyond that, there's this discussion: conda-forge/conda-forge.github.io#1844

hmaarrfk · 2023-02-04T16:47:00Z

While you are using the newer SDK, you are still targetting the older deployment targets unless you specify one more variable.

https://conda-forge.org/docs/maintainer/knowledge_base.html#requiring-newer-macos-sdks

hmaarrfk · 2023-02-04T17:50:56Z

see conda-forge/conda-forge-ci-setup-feedstock#228 (comment)

ngam · 2023-02-04T20:13:44Z

Yeah, I understand the difference between the deployment target and the SDK. I am just not sure why we set the SDK so low if we can set it higher with no downsides. If I remember, I think isuruf said that it’s not guaranteed that it would work across the board, so we are just being safe in setting it lower.

I saw the SDK fix. I would recommend to keep the pytorch feedstock at the highest SDK we could, because they’ve been adding all sorts of newer functionalities for MPS that might have appeared in later releases. But I personally don’t care as much at the moment. So I am happy to revert this PR.

(A discussion for another day: The larger issue around MacOS support is that Apple is quite unfriendly to those who don’t update/upgrade. On Linux, it makes sense we go out of our way to support older systems (believe me, I am on some “newer” HPC systems that have git 1.8.x and python 2.7 as defaults). There are these random stats that latest iOS/macOS adoption/upgrade rate is more than 90%. The bottom line, we could potentially make our lives easier by going with the newer stuff. But if it is not broken, why bother?! And it definitely isn’t broken 😄)

isuruf · 2023-02-04T20:17:02Z

I am just not sure why we set the SDK so low if we can set it higher with no downsides.

There are downsides. Most build systems don't understand the difference between the two (SDK version vs deployment target).
Even for a prominent package like python, only python 3.9+ understand the difference whereas for python 3.8, the build system assumes that the depolyment target and the SDK version are the same.

hmaarrfk · 2023-02-05T14:48:08Z

recipe/meta.yaml

@@ -100,12 +102,7 @@ outputs:
        - {{ pin_compatible('magma', max_pin='x.x.x') }}      # [cuda_compiler_version != "None"]
        # other requirements
        - python
-        - {{ pin_compatible('numpy') }}


if we cannot remove numpy from the build time dependneices, this should be moved to a run_constraint

Sorry, I misunderstood. Do you want us to remove them as build dependency or just run dependencies? I thought run dependencies only...

https://github.com/pytorch/pytorch/tree/60a3b7425dde97fe8b46183c154a9c3b24f0c733/.github/requirements

Their package doesn't seem to depend on at runtime

Their recipe seems to look like:

{% set build_variant = environ.get('PYTORCH_BUILD_VARIANT', 'cuda') %} {% set cross_compile_arm64 = environ.get('CROSS_COMPILE_ARM64', 0) %} package: name: pytorch version: "{{ environ.get('PYTORCH_BUILD_VERSION') }}" source: path: "{{ environ.get('PYTORCH_GITHUB_ROOT_DIR') }}" requirements: build: - cmake - {{ compiler('c') }} # [win] - pkg-config # [unix] - libuv # [unix] host: - python - setuptools - pyyaml {% if cross_compile_arm64 == 0 %} - mkl-include # [x86_64] - mkl=2020.2 # [x86_64 and (not win or py <= 39)] - mkl=2021.4 # [x86_64 and win and py >= 310] {% endif %} - typing_extensions - ninja - libuv # [win] - numpy=1.19 # [py <= 39] - numpy=1.21.5 # [py >= 310] - openssl=1.1.1l # [py >= 310 and linux] {{ environ.get('PYTORCH_LLVM_PACKAGE', ' - llvmdev=9') }} {{ environ.get('MAGMA_PACKAGE', '') }} run: - python {% if cross_compile_arm64 == 0 %} - mkl >=2018 # [x86_64] {% endif %} - libuv # [win] - intel-openmp # [win] - typing_extensions {% if cross_compile_arm64 == 0 %} - blas * mkl {% endif %} - pytorch-mutex 1.0 {{ build_variant }} # [not osx ] {{ environ.get('CONDA_CUDATOOLKIT_CONSTRAINT', '') }} {% if build_variant == 'cpu' %} run_constrained: - cpuonly {% elif not osx %} run_constrained: - cpuonly <0 {% endif %} build: number: {{ environ.get('PYTORCH_BUILD_NUMBER', '1') }} detect_binary_files_with_prefix: False string: "{{ environ.get('PYTORCH_BUILD_STRING') }}" script_env: - BUILD_SPLIT_CUDA - CUDA_VERSION - CUDNN_VERSION - CONDA_CUDATOOLKIT_CONSTRAINT - USE_CUDA - CMAKE_ARGS - EXTRA_CAFFE2_CMAKE_FLAGS - DEVELOPER_DIR - DEBUG - USE_FBGEMM - USE_GLOO_WITH_OPENSSL # [unix] - USE_SCCACHE # [win] - USE_DISTRIBUTED # [unix] - CMAKE_OSX_ARCHITECTURES # [unix] - USE_MKLDNN # [unix] - USE_NNPACK # [unix] - USE_QNNPACK # [unix] - BUILD_TEST # [unix] - USE_PYTORCH_METAL_EXPORT # [osx] - USE_COREML_DELEGATE # [osx] - _GLIBCXX_USE_CXX11_ABI # [unix] test: imports: - torch about: home: http://pytorch.org/ license: BSD 3-Clause license_family: BSD license_file: LICENSE summary: PyTorch is an optimized tensor library for deep learning using GPUs and CPUs.

Which means that numpy is required at build time, but not runtime. Therefore, we should move the line you added to the run_constraint.

It seems we can just remove numpy from the run section. Which one?

add numpy as run_constraint

add llvm-openmp as run_constraint instead of run

both?

I'm not an expert with llvm. I can't comment on that.

ngam · 2023-02-05T15:59:29Z

upstream they use Xcode 13.3.1 which corresponds to SDK 12.3

Will revert to that.

…nda-forge-pinning 2023.02.04.12.26.59

ngam · 2023-02-05T16:49:07Z

PR upcoming in other feedstock to fix latest issue... conda-forge/conda-forge-ci-setup-feedstock#230

hmaarrfk · 2023-02-05T23:40:34Z

gah sorry conda-forge/conda-forge-ci-setup-feedstock#231

ngam · 2023-02-05T23:49:28Z

No problem... btw, I confirmed that for both cases, the actual folders do have the .sdk suffix, so what you did was arguably more coherent 😺

ngam · 2023-02-06T23:07:47Z

Looks good to me; @hmaarrfk what do you think?

Paging @Tobias-Fischer as well

ngam · 2023-02-06T23:14:33Z

(btw, I wouldn't bother uploading cuda builds...)

Tobias-Fischer · 2023-02-07T02:27:30Z

Awesome, thanks for that! Seems like you got the osx-* build all to pass which is great :)

hmaarrfk · 2023-02-07T02:31:42Z

recipe/meta.yaml

        - mkl-devel {{ mkl }}   # [x86]
        - libcblas * *_mkl      # [x86]
        - libcblas              # [not x86]
        - liblapack             # [not x86]
        - openblas              # [not x86]
+        - libgomp   # [linux]


i was only going to ask about this line with respect to the CUDA builds.

I added this to be consistent with the osx additions. However, I highly doubt it is worth the hassle of building locally and uploading. We never faced any issues with the cuda builds. So yes, imo we could skip building cuda this time around; if there are any problems, we will address them in the next release, and perhaps make a more informed decision about adding/removing this. But ultimately you will have to make a decision on this...

ok lets skip the builds this time, and maybe upload them over time later

ninja was already removed as a dependency so i htink the main issue is resolved via a repo-data patch

Sounds good. I would offer to build them myself, but I just moved jobs (with plenty of access to HPCs) and I need time to navigate the policies 😶‍🌫️

Congrats on the move!

Tobias-Fischer · 2023-02-07T11:44:01Z

Can confirm this PR fixed conda-forge/norse-feedstock#11

hmaarrfk · 2023-02-07T12:38:13Z

great!

ngam added 3 commits February 2, 2023 20:56

add llvm-opnemp to host + move up osx sdk due to recent troubles

3ba429d

build number

4f00f13

remove ninja as per issue

23f624b

MNT: Re-rendered with conda-build 3.23.3, conda-smithy 3.22.1, and co…

d73307f

…nda-forge-pinning 2023.02.02.19.14.40

hmaarrfk reviewed Feb 3, 2023

View reviewed changes

recipe/meta.yaml Outdated Show resolved Hide resolved

delete deps ...

918746c

hmaarrfk reviewed Feb 5, 2023

View reviewed changes

ngam and others added 2 commits February 5, 2023 11:37

remove cffi and go back to 12.3

2bd8c1a

MNT: Re-rendered with conda-build 3.23.3, conda-smithy 3.22.1, and co…

6b10a3c

…nda-forge-pinning 2023.02.04.12.26.59

ngam mentioned this pull request Feb 5, 2023

fix untarring osx SDKSs from recent PR conda-forge/conda-forge-ci-setup-feedstock#230

Merged

5 tasks

hmaarrfk closed this Feb 5, 2023

hmaarrfk reopened this Feb 5, 2023

hmaarrfk closed this Feb 6, 2023

hmaarrfk reopened this Feb 6, 2023

hmaarrfk reviewed Feb 7, 2023

View reviewed changes

hmaarrfk approved these changes Feb 7, 2023

View reviewed changes

ngam marked this pull request as ready for review February 7, 2023 03:08

ngam requested review from benjaminrwilson and sodre as code owners February 7, 2023 03:08

hmaarrfk merged commit c48cd2b into conda-forge:main Feb 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps cleanup #162

deps cleanup #162

ngam commented Feb 3, 2023 •

edited

Loading

conda-forge-webservices bot commented Feb 3, 2023

ngam commented Feb 3, 2023 •

edited

Loading

hmaarrfk commented Feb 3, 2023

ngam commented Feb 3, 2023 •

edited

Loading

h-vetinari commented Feb 3, 2023

hmaarrfk commented Feb 4, 2023

hmaarrfk commented Feb 4, 2023

ngam commented Feb 4, 2023

isuruf commented Feb 4, 2023

hmaarrfk Feb 5, 2023

ngam Feb 5, 2023

ngam Feb 5, 2023

hmaarrfk Feb 5, 2023

ngam Feb 5, 2023 •

edited

Loading

hmaarrfk Feb 5, 2023

ngam commented Feb 5, 2023 •

edited

Loading

ngam commented Feb 5, 2023 •

edited

Loading

hmaarrfk commented Feb 5, 2023

ngam commented Feb 5, 2023 •

edited

Loading

ngam commented Feb 6, 2023

ngam commented Feb 6, 2023

Tobias-Fischer commented Feb 7, 2023

hmaarrfk Feb 7, 2023

ngam Feb 7, 2023

hmaarrfk Feb 7, 2023

hmaarrfk Feb 7, 2023

ngam Feb 7, 2023

hmaarrfk Feb 7, 2023

Tobias-Fischer commented Feb 7, 2023

hmaarrfk commented Feb 7, 2023

deps cleanup #162

deps cleanup #162

Conversation

ngam commented Feb 3, 2023 • edited Loading

conda-forge-webservices bot commented Feb 3, 2023

ngam commented Feb 3, 2023 • edited Loading

hmaarrfk commented Feb 3, 2023

ngam commented Feb 3, 2023 • edited Loading

h-vetinari commented Feb 3, 2023

hmaarrfk commented Feb 4, 2023

hmaarrfk commented Feb 4, 2023

ngam commented Feb 4, 2023

isuruf commented Feb 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngam Feb 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngam commented Feb 5, 2023 • edited Loading

ngam commented Feb 5, 2023 • edited Loading

hmaarrfk commented Feb 5, 2023

ngam commented Feb 5, 2023 • edited Loading

ngam commented Feb 6, 2023

ngam commented Feb 6, 2023

Tobias-Fischer commented Feb 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobias-Fischer commented Feb 7, 2023

hmaarrfk commented Feb 7, 2023

ngam commented Feb 3, 2023 •

edited

Loading

ngam commented Feb 3, 2023 •

edited

Loading

ngam commented Feb 3, 2023 •

edited

Loading

ngam Feb 5, 2023 •

edited

Loading

ngam commented Feb 5, 2023 •

edited

Loading

ngam commented Feb 5, 2023 •

edited

Loading

ngam commented Feb 5, 2023 •

edited

Loading