Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
github-actions merged 1 commit into from
Jan 2, 2021
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion recipe/meta.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
{% set version = "3.9.0" %}
# if build_num is reset to 0 (for new version), update increment for blas_minor below
{% set build_num = 5 %}

@isuruf isuruf Jan 2, 2021 •
edited
Loading

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

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

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

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

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.

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

Thanks, done.

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

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

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

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.

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

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

@h-vetinari h-vetinari Jan 2, 2021 •
edited
Loading

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

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

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

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

Happy to hear it! :)

{% set version_major = version.split(".")[0] %}
{% set blas_major = "2" %}
# make sure we do not create colliding version strings of output "blas"
# for builds across lapack-versions within the same blas_major
{% set blas_minor = build_num + 100 %}

# blas_major denotes major infrastructural change to how blas is managed
package:
Expand Down Expand Up @@ -178,7 +182,7 @@ outputs:

# For compatiblity
- name: blas
version: "{{ blas_major }}.{{ build_num }}"
version: "{{ blas_major }}.{{ blas_minor }}"
script: test_blas.sh # [unix]
script: test_blas.bat # [win]
build:
Expand Down Expand Up @@ -220,6 +224,7 @@ outputs:
- if not exist %LIBRARY_BIN%/liblapacke.dll exit 1 # [win]

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

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

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.

build:
string: "{{ build_num }}_{{ blas_impl }}"
requirements:
Expand Down