Skip to content

ensure versions for output 'blas' don't overlap#60

Merged
github-actions[bot] merged 1 commit into
conda-forge:masterfrom
h-vetinari:master
Jan 2, 2021
Merged

ensure versions for output 'blas' don't overlap#60
github-actions[bot] merged 1 commit into
conda-forge:masterfrom
h-vetinari:master

Conversation

@h-vetinari

Copy link
Copy Markdown
Member

The versions strings for output blas were colliding with the ones that had been produced for lapack 3.8.0 (at least). Disambiguate by bumping the number that goes into those version strings.

@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 commented Jan 2, 2021

Copy link
Copy Markdown
Member Author

@isuruf

After #59, conda create -n test python numpy "blas=*=blis" does not work as expected, and I believe the version numbering of the blas-output is the issue. The above command pulls in:

The following NEW packages will be INSTALLED:

  blas               conda-forge/win-64::blas-2.3-blis
  blis               conda-forge/win-64::blis-0.7.0-h62dcd97_1
  [...]
  libblas            conda-forge/win-64::libblas-3.9.0-3_blis
  libcblas           conda-forge/win-64::libcblas-3.9.0-3_blis
  liblapack          conda-forge/win-64::liblapack-3.9.0-3_hd5c7e75_netlib
  liblapacke         conda-forge/win-64::liblapacke-3.9.0-3_hd5c7e75_netlib

On the other hand, conda create -n test python numpy "libblas=*=5_blis" works and pulls in:

The following NEW packages will be INSTALLED:

  blis               conda-forge/win-64::blis-0.8.0-h8d14728_0
  [...]
  libblas            conda-forge/win-64::libblas-3.9.0-5_blis
  libcblas           conda-forge/win-64::libcblas-3.9.0-5_blis
  liblapack          conda-forge/win-64::liblapack-3.9.0-3_hd5c7e75_netlib

Comment thread recipe/meta.yaml
@@ -2,6 +2,10 @@
{% set build_num = 5 %}

@isuruf isuruf Jan 2, 2021

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.

Let's change this to 22 and make a comment that it should not be reset to 0

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.

It's not by a big margin, but I don't think that's a better solution, for two reasons:

  • new lapack versions should IMO reset the build number (aside from being confusing for CF-regulars, the GH PR template invites to), and there's really two different constructs being versioned here (lapack & CF-blas-arch), which don't have to have the same build_num
  • in case there's ever a need to create a branch for rebuilding for an older lapack-version, it would be very intransparent what the right build_num should be

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.

I don't really agree with 1., but 2 is good enough. Can you add a comment above that if it is reset, then blas_minor offset needs to be updated.

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.

Thanks, done.

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.

As a matter of fact, what do you think about creating a 3.8 branch here and we do one more build that has a consistent build number & corresponding blas-version? Because currently, there's a bit of an eclectic mix with blas builds for 3.9 but with low numbers

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.

It'll still be hard to follow as well. netlib builds are done with a different build number as well and I don't think we'll be able to keep the build numbers in sync for different blas implementations and IMO that's okay.

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.

Sure, but the netlib ones don't have an associated blas package.

@h-vetinari h-vetinari Jan 2, 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.

Nevermind, they do. OK, I'll drop the argument for a 3.8 rebuild, thanks for the exchange

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.

Thanks for the discussion. I really appreciate your work on better BLAS support on conda-forge.

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.

Happy to hear it! :)

@isuruf isuruf added the automerge Merge the PR when CI passes label Jan 2, 2021
Comment thread recipe/meta.yaml
- if not exist %LIBRARY_BIN%/liblapacke.dll exit 1 # [win]

- name: blas-devel
# uses lapack {{ version }}, not {{ blas_major }}

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 was double-checking this, because it's not immediately clear from the recipe what's happening here. To be honest, I was surprised that blas-devel is using a different versioning scheme than blas, but whether intentional or not, it probably can't be fixed or changed until blas_major overtakes the lapack version. In the meantime, I thought it's better to leave a comment.

@github-actions github-actions Bot merged commit b07547a into conda-forge:master Jan 2, 2021
@github-actions

github-actions Bot commented Jan 2, 2021

Copy link
Copy Markdown
Contributor

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

  • linter: passed
  • drone: passed
  • azure: passed

Thus the PR was passing and merged! Have a great day!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge Merge the PR when CI passes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants