Skip to content

run test-suite (and repack netlib) for blis#62

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

run test-suite (and repack netlib) for blis#62
h-vetinari wants to merge 8 commits into
conda-forge:masterfrom
h-vetinari:master

Conversation

@h-vetinari

Copy link
Copy Markdown
Member

No description provided.

@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.

@h-vetinari h-vetinari marked this pull request as draft January 2, 2021 14:21
@h-vetinari

Copy link
Copy Markdown
Member Author

Getting an error

  File "/opt/conda/lib/python3.8/site-packages/conda_build/render.py", line 475, in _simplify_to_exact_constraints
    raise ValueError("Conflicting exact pins: {}".format(exact_pins))
ValueError: Conflicting exact pins: [['3.9.0', '3_h92ddd45_netlib'], ['3.9.0', '3_h92ddd45_netlib'], ['3.9.0', '6_blis']]

Based on the number of resolution steps before, I'm guessing this happens for blas-devel. Let's see if d80fe2b helps.

@conda-forge-linter

Copy link
Copy Markdown

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but it appears we have a merge conflict.
Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

@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.

@isuruf

isuruf commented Jan 7, 2021

Copy link
Copy Markdown
Member

FWIW, I don't like repackaging the netlib variant of lapack as blis. With the current scheme, it's obvious that we are using BLAS from blis and LAPACK from netlib. With the new scheme, it's not obvious whether LAPACK comes from netlib or flame or other source.

@h-vetinari

Copy link
Copy Markdown
Member Author

FWIW, I don't like repackaging the netlib variant of lapack as blis. With the current scheme, it's obvious that we are using BLAS from blis and LAPACK from netlib. With the new scheme, it's not obvious whether LAPACK comes from netlib or flame or other source.

It's a point I expected to discuss with you. ;-)

But right now I was just trying to figure out how to run the test suite for blis here... Not sure if conda could be taught to correctly resolve the exact subpackage pins mixed with other variants, but so far, the only thing I've gotten working is a repack.

I had an intermediate version (lost since the rebase) of renaming the lapack(e)-outputs for blis only, e.g. it could be something like lapack(e)-3.9.0-7_netlib_repacked_for_blis, which would still work with the *blis selector. WDYT?

@h-vetinari

Copy link
Copy Markdown
Member Author

Also, I now have a running test suite for blis on windows, and the results are bad: 13% tests passed, 90 tests failed out of 103. This probably explains why I'm having ~440 errors in the scipy test suite for blis on windows.

@h-vetinari

Copy link
Copy Markdown
Member Author

@h-vetinari: Also, I now have a running test suite for blis on windows, and the results are bad: 13% tests passed, 90 tests failed out of 103. This probably explains why I'm having ~440 errors in the scipy test suite for blis on windows.

On second thought - all the BLAS* tests pass, all the LAPACK* tests fail - turns out the integration isn't working yet after all...

@h-vetinari h-vetinari marked this pull request as ready for review January 9, 2021 01:36
@h-vetinari h-vetinari changed the title WIP: try running test-suite for blis run test-suite (and repack netlib) for blis Jan 9, 2021

@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.

@isuruf, this should be ready for a first round of discussions - not saying repacking is a must, but I believe (with the build string distinction) it's definitely feasible without ambiguity.

Comment thread recipe/meta.yaml
Comment on lines -201 to +206
- {{ pin_subpackage("blas-devel", exact=True) }}
- {{ pin_subpackage("blas", exact=True) }}

@h-vetinari h-vetinari Jan 9, 2021

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.

@isuruf, not sure I understood your intention from #64 here correctly, but blas-devel depending on itself sounds fragile and circular. My understanding would be that it builds on blas and adds the "development headers" (or whatever is the equivalent)...?

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.

Yes that's a mistake and should be removed. blas should depend on blas-devel.

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.

I thought it was the other way around? Kinda like blas-devel > blas (in terms of dependencies)

At least that's how I've understood the -dev[el] packages in various distros, for example.

Comment thread recipe/meta.yaml
- liblapacke {{ version }} *netlib # [blas_impl == 'blis']
- {{ pin_subpackage("libcblas", exact=True) }}
- {{ pin_subpackage("libblas", exact=True) }}
- {{ pin_subpackage("blas-devel", exact=True) }}

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, not sure why blas should need e.g. mkl-devel?

@h-vetinari

Copy link
Copy Markdown
Member Author

Ping @isuruf :)

@h-vetinari

Copy link
Copy Markdown
Member Author

@isuruf
WDYT about linux-64/liblapack-3.9.0-8_netlib_repacked_for_blis.tar.bz2?

@h-vetinari

Copy link
Copy Markdown
Member Author

@isuruf
I'd like to merge this - I believe I have addressed your concerns, but would like to have confirmation from (or discussion with) you to be sure.

@isuruf

isuruf commented Jan 26, 2021

Copy link
Copy Markdown
Member

I'm not a fan of this. I believe we can test this by creating the symlinks for blis in the testing script. I'll have a look, but will not be soon.

@h-vetinari

Copy link
Copy Markdown
Member Author

ok, thanks for the response. If you can outline your plan (e.g. symlink what to where), I can give it a shot. In the meantime, I'm placing this PR in draft mode.

@h-vetinari h-vetinari marked this pull request as draft January 26, 2021 19:34
@isuruf

isuruf commented Jan 26, 2021

Copy link
Copy Markdown
Member

Thanks @h-vetinari for all your efforts.

I think what we need to do is in test_blas.sh call build_pkg.sh for blis four times with PKG_NAME set for blas, cblas, lapack, lapacke.

@h-vetinari

Copy link
Copy Markdown
Member Author

OK, I'll think about it and might give it a shot. While we're at it, could you briefly comment on this? Circular dependencies sound bad to me, and should probably be fixed ASAP independently of the blis-stuff.

@h-vetinari

Copy link
Copy Markdown
Member Author

Obsoleted by #65

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