Release 1.10.1 and migrate protobuf and mkl #84
Conversation
|
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 ( |
|
@hmaarrfk, could you please test one minor change? Could you comment out this line? if [[ "$target_platform" == "osx-arm64" ]]; then
export BLAS=OpenBLAS
export USE_MKLDNN=0
# There is a problem with pkg-config
# See https://github.com/conda-forge/pkg-config-feedstock/issues/38
export USE_DISTRIBUTED=0
fiThe motivation is, cmake will likely look for whatever is available and you have OpenBLAS included, so it would fall back on it anyway. However, it would likely pick up Apple's Accelerate instead of OpenBLAS. Since this seems to be passing, I will test it locally Edit: not quickly, but I will try to report back. Feel free to proceed without this though for now. Nothing urgent. |
|
As expected, commenting out that line (forcing it to be OpenBLAS) changes the CMAKE process from: to: |
|
So, this happens despite the fact that you have OpenBLAS available. Why? Because OpenBLAS is considered a fail-safe basically. It seems to me that upstream people (not just here) prefer Accelerate over OpenBLAS --- they much prefer MKL, as seen above. I guess we could use BLIS as well... |
|
And finally: from: to: |
|
Note though that it is using Accelerate's LAPACK. |
|
I'm going to refer to the comment made #83 (comment) I would suggest, that for this kind of optimization, that you demonstrate some real gains to be had by moving to accelerate. Maybe you can provide a benchmark showing the improvement? It is pretty easy to build things yourself and upload them to your channel. I strongly suggest that instead of waiting too long on the community for your own work anyway. once we've established the gains, we can work on integrating it here |
|
Again, OSX optimizations here are "easy" in the sense that they follow the standard conda-forge model of building things on shared CIs. We can always "delay" for an other small PR and avoid rebuilding the linux builds there. |
|
hmmm. the problem might be that we pinned too hard. we will need a repo patch to update packages to be a little looser. |
I guess it should be the other way around --- you don't have to set I would argue the following: By doing this sort of pick-and-choose and ignoring the wishes of upstream developers, you're essentially making a customized fork of pytorch and thus should be renamed to cftorch or something. This is not about the improvements per se, but about the philosophy of redistribution. Aren't we supposed to keep this as close to standard-as-upstream as possible? If that's the case, I don't get why we have an interventionist environment variable. I think there's a fetishizing going on by wanting to always have OpenBLAS as the common denominator; that's a harmful strategy.
There is no work. It is literally just commenting out that line. if [[ "$target_platform" == "osx-arm64" ]]; then
- export BLAS=OpenBLAS
+ # no need to force building again OpenBLAS
+ # cmake can figure out default config from upstream
+ # export BLAS=OpenBLAS
export USE_MKLDNN=0
# There is a problem with pkg-config
# See https://github.com/conda-forge/pkg-config-feedstock/issues/38
export USE_DISTRIBUTED=0
fi
|
|
Also that @isuruf's comment is unsupported. Why would all these developers refer to accelerate by default then? Also, Apple still uses and supports Accelerate; it is in their dev documentation about BNNS and the like. |
|
i totally understand all the points above. The good news for you, is that this is an isolated request, that we can focus on after this. Merging an OSX patch into CF is easy since it all runs on the bots. You decided to backout of the other isolated patch you are doing. I don't think I have the bandwidth to iterate on OSX in this merge request. Had to rebuild for loosening the pinnings already. Lets just try to focus on one thing at a time. |
|
And we are building a "cf-pytorch". Upstream developers are seldom interested in integrating in the larger ecosystem. They are interested in their business demos, and not in the larger system. cf is about making it easy to install many libraries at the same time. Something that is impossible with just the "pytorch" channel. You can see how instead of using a central library system, pytorch vendors many libraries that could be shared libraries increasing build complexity. In either way, this is best disucssed in a separate issue. |
Yes, that's fine. This osx stuff is very minor compared to the other work you're doing, so unless you have an opening where everything is going smoothy, ignore it for now and we can revisit.
Yes, but if we deviate a lot, we should rename this. Like I've been suggesting with julia due to the deviation. |
Again, we are happy to review the requests and isuruf provided many good inputs. OSX updates can be merged independently. It is the linux (and windows) builds that take the longest since they require manual intervention. |
|
@hmaarrfk for your consideration re MKL update: pytorch/pytorch#68812 (comment) Edit: keep an eye for osx failures, pytorch/pytorch@a07d3dc |
|
Also, @hmaarrfk I forgot to mention this earlier. When I was compiling for cuda builds for mxnet, I noticed a deprecation warning: In this case, to decrease compile time, I would remove this line https://github.com/hmaarrfk/pytorch-cpu-feedstock/blob/23612dc7e4b69545153bf66e37a3423089997108/recipe/build_pytorch.sh#L93, thought this may need more coordination across the "ecosystem" ... |
|
I think I'm going to keep all the architectures for now for the CUDA versions I get around to. are you asking me to include a patch? I might be able to at this stage. Please try to keep this discussion focused on the improvement at hand. otherwise we will never get anything released. Happy to have larger discussions in the issues |
|
No, we will figure out a better way to handle the BLAS stuff later. I just came across that issue with MKL and I thought I would flag it to you. The title of this PR has "MKL" in it. For the CUDA builds, that's just a thought about excessive build times --- it is also relevant here in general, since the builds are timing out. However, I can't actually see how far they go; if it is around 80--90%, then removing the additional deprecated cuda archs might make them conclude in time, in under 6 hours. |
|
For 10.2, it's timing out at 4935/5485, I don't think removing 3--5 will help, but maybe --- just maybe --- removing 3--5 and 6.1 will make the cut |
|
The issue is that on my machine, i get to 4900 in 1 hour, and the remainder takes 1 hour. So I'm not sure how to cut down on the compilation time. We would need to cut down by a factor of 2. Ok, since there is no action item, i'm going to keep pushing on building this. |
Yes, keep going :) Thanks! |
hmaarrfk
force-pushed
the
release_1.10.1
branch
from
23612dc
to
82edc97
Compare
|
I had to rerender, but i'm about 10/12 builds through. I believe the file contents in ci_support are the same. but the commit message will have changed. I'll let whoever reviews decide on the course of action. I likely won't have an other machine for 24+ hours to build all the builds. |
…nda-forge-pinning 2022.01.25.13.00.06
hmaarrfk
force-pushed
the
release_1.10.1
branch
from
82edc97
to
a8870c0
Compare
|
The builds were made before rebasing: $ git diff --compact-summary release_1.10.1 origin/release_1.10.1
.azure-pipelines/azure-pipelines-linux.yml | 90 +++++++++++++++++++++++++++++++++++++++---------------------------------------
...n3.7.____cpython.yaml => linux_64_c_compiler_version7cuda_compiler_version10.2cudnn7cxx_compiler_version7numpy1.18python3.7.____cpython.yaml} | 0
...n3.8.____cpython.yaml => linux_64_c_compiler_version7cuda_compiler_version10.2cudnn7cxx_compiler_version7numpy1.18python3.8.____cpython.yaml} | 0
...n3.9.____cpython.yaml => linux_64_c_compiler_version7cuda_compiler_version10.2cudnn7cxx_compiler_version7numpy1.19python3.9.____cpython.yaml} | 0
...n3.7.____cpython.yaml => linux_64_c_compiler_version9cuda_compiler_version11.0cudnn8cxx_compiler_version9numpy1.18python3.7.____cpython.yaml} | 0
...n3.8.____cpython.yaml => linux_64_c_compiler_version9cuda_compiler_version11.0cudnn8cxx_compiler_version9numpy1.18python3.8.____cpython.yaml} | 0
...n3.9.____cpython.yaml => linux_64_c_compiler_version9cuda_compiler_version11.0cudnn8cxx_compiler_version9numpy1.19python3.9.____cpython.yaml} | 0
...n3.7.____cpython.yaml => linux_64_c_compiler_version9cuda_compiler_version11.1cudnn8cxx_compiler_version9numpy1.18python3.7.____cpython.yaml} | 0
...n3.8.____cpython.yaml => linux_64_c_compiler_version9cuda_compiler_version11.1cudnn8cxx_compiler_version9numpy1.18python3.8.____cpython.yaml} | 0
...n3.9.____cpython.yaml => linux_64_c_compiler_version9cuda_compiler_version11.1cudnn8cxx_compiler_version9numpy1.19python3.9.____cpython.yaml} | 0
...n3.7.____cpython.yaml => linux_64_c_compiler_version9cuda_compiler_version11.2cudnn8cxx_compiler_version9numpy1.18python3.7.____cpython.yaml} | 0
...n3.8.____cpython.yaml => linux_64_c_compiler_version9cuda_compiler_version11.2cudnn8cxx_compiler_version9numpy1.18python3.8.____cpython.yaml} | 0
...n3.9.____cpython.yaml => linux_64_c_compiler_version9cuda_compiler_version11.2cudnn8cxx_compiler_version9numpy1.19python3.9.____cpython.yaml} | 0
..._cpython.yaml => linux_64_c_compiler_version9cuda_compiler_versionNonecudnnundefinedcxx_compiler_version9numpy1.18python3.7.____cpython.yaml} | 0
..._cpython.yaml => linux_64_c_compiler_version9cuda_compiler_versionNonecudnnundefinedcxx_compiler_version9numpy1.18python3.8.____cpython.yaml} | 0
..._cpython.yaml => linux_64_c_compiler_version9cuda_compiler_versionNonecudnnundefinedcxx_compiler_version9numpy1.19python3.9.____cpython.yaml} | 0
README.md | 60 ++++++++++++++++++++++++++--------------------------
conda-forge.yml | 7 +++---
18 files changed, 79 insertions(+), 78 deletions(-)I think we will be OK to upload, full diff text can be found: |
|
Logs available: The uploads are found under the unique label cfep03 @isuruf if you could upload that would be greatly appreciated. I thought that hte builds were taking time to upload, then I realized that pytorch's builds are even bigger than ours at 1.5GB/build vs 900MB/build for us. edit: please wait an hour or so for the builds to finish uploading. |
|
It seems that there are no pytorch-cpu builds. |
|
those have been successfully completing on the CIs. |
|
Thank you! |
|
Can you open an issue in https://github.com/conda-forge/conda-forge.github.io ? |
|
Sure, let me dig around a little more to get all the details right |
Checklist
0(if the version changed)conda-smithy(Use the phrase@conda-forge-admin, please rerenderin a comment in this PR for automated rerendering)