Skip to content

Build for 3.9.0#43

Closed
h-vetinari wants to merge 5 commits into
conda-forge:masterfrom
h-vetinari:master
Closed

Build for 3.9.0#43
h-vetinari wants to merge 5 commits into
conda-forge:masterfrom
h-vetinari:master

Conversation

@h-vetinari

Copy link
Copy Markdown
Member

Checklist

  • Used a 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.

Version 3.9.0 has been released for a while, let's try building for it.

@conda-forge-admin, please rerender

@conda-forge-linter

Copy link
Copy Markdown

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.

@conda-forge-linter

Copy link
Copy Markdown

Hi! This is the friendly automated conda-forge-webservice.

I tried to re-render for you but ran into some issues, please ping conda-forge/core for further assistance. You can also try re-rendering locally.

@h-vetinari h-vetinari left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Comments on the changes

Comment thread recipe/meta.yaml

source:
url: http://www.netlib.org/lapack/lapack-{{ version }}.tar.gz
sha256: deb22cc4a6120bff72621155a9917f485f96ef8319ac074a7afbc68aab88bcf6

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The url for the sources changed between 3.8.0 and 3.9.0

Comment thread recipe/meta.yaml
- cmake
- m2-make # [win]
- posix # [win]
# hack (around which bug?)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was noted as a hack in a5df20d, can we document what's necessary to remove it?

Comment thread recipe/meta.yaml
- llvm-openmp # [linux and blas_impl == "openblas"]
host:
# Building with blis fails due to a conda-build bug
# Building with blis fails due to a conda-build bug - which one?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Same here - which conda-build bug is relevant here? What would be necessary to simply use:

      host:
        - {{ pin_subpackage("libblas", exact=True) }}
        - {{ pin_subpackage("libcblas", exact=True) }}
      {% if blas_impl != 'blis' %}
        - {{ pin_subpackage("liblapack", exact=True) }}
        - {{ pin_subpackage("liblapacke", exact=True) }}
      {% endif %}

@h-vetinari

h-vetinari commented Mar 3, 2020

Copy link
Copy Markdown
Member Author

OK, clearly I misunderstood this recipe, and how it's necessary to build https://github.com/conda-forge/lapack-feedstock for 3.9.0 first.

Edit: conda-forge/lapack-feedstock#32
Edit2: also note that blis=0.6.1 failed to build for windows after merging the PR in the feedstock. See conda-forge/blis-feedstock#12

@h-vetinari

h-vetinari commented Apr 26, 2020

Copy link
Copy Markdown
Member Author

I rebased this in anticipation of conda-forge/lapack-feedstock#32 hopefully being finished soon.

I have to say that I really had trouble wrapping my head around this recipe. I couldn't understand (from the meta.yaml) how libblas depends on the netlib implementation:

PackagesNotFoundError: The following packages are not available from current channels:

  - libcblas==3.9.0[build=*netlib]
  - liblapacke==3.9.0[build=*netlib]
  - liblapack==3.9.0[build=*netlib]
  - libblas==3.9.0[build=*netlib]

It took me a while to find out that that dependency only appears in build.sh (which also gets called in bld.bat, but curiously doesn't error the build upon failing).

Also, why do we download the netlib-sources again if the build is already done in the feedstock for lapack? I'm thinking this could/should be made more explicit, no? Will try for a patch an explanatory comment...

Edit: figured out why we need the sources at least (linking & testing)
Edit2: I think the penny may finally be dropping. 😅
netlib must be installed in a separate env to link from during the build; which can then be replaced at runtime by another implementation.

@h-vetinari

h-vetinari commented Apr 26, 2020

Copy link
Copy Markdown
Member Author

OK, I've had a few pennies drop (see edits above), but now that I was trying to put this into words, I still have to wonder:

Why do we link against netlib at all if build_pkg.(sh|bat) will immediately replace it? Yes, there is a testrun of the netlib-packages being performed, but why not do that (including more_testing.diff) in the lapack-feedstock?

What am I (still) missing @isuruf?

@h-vetinari

Copy link
Copy Markdown
Member Author

On another note: After some tries, 6b4fbef now correctly aborts windows builds on errors within build.sh as well.

@isuruf

isuruf commented Apr 26, 2020

Copy link
Copy Markdown
Member

For testing, what we do here is, we build the test-suite of netlib and then we replace the implementation with openblas or blis or mkl and run the test suite to see if changing the blas implementation under the hood actually does work.

@h-vetinari

Copy link
Copy Markdown
Member Author

For testing, what we do here is, we build the test-suite of netlib and then we replace the implementation with openblas or blis or mkl and run the test suite to see if changing the blas implementation under the hood actually does work.

Understood, my point was that the test suite for the netlib implementation is already run in the lapack-feestock, and just running the tests here with openblas or blis or mkl should be enough, no?

@isuruf

isuruf commented Apr 26, 2020

Copy link
Copy Markdown
Member

We are running the netlib test suite with openblas, etc. (We aren't running netlib test suite with netlib) This is done to check that these implementations are indeed compatible.

@h-vetinari

Copy link
Copy Markdown
Member Author

OK, that makes sense! Thanks for the explanation.

@h-vetinari

Copy link
Copy Markdown
Member Author

@isuruf
I tried to put what I understood in an explanatory comment (see 316649a). Happy to iterate if I'm still not getting something, but I'd hope that this can be included so that the next person looking at this recipe might be less lost than I am. ;-)

@h-vetinari

Copy link
Copy Markdown
Member Author

Superseded by #52.

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