Skip to content
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.

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

[staging-20.09] openblas: 0.3.10 -> 0.3.12 #108709

Merged
merged 1 commit into from Feb 8, 2021

Conversation

nomeata
Copy link
Contributor

@nomeata nomeata commented Jan 7, 2021

this backports the following commits from master to 20.09:

The motivation is to unbreak building python36Packages.scipy (see
issue 92458)

@nomeata
Copy link
Contributor Author

nomeata commented Jan 7, 2021

(Targeting staging as per advice by @TredwellGit in #92458 (comment))

@nomeata
Copy link
Contributor Author

nomeata commented Jan 7, 2021

@ofborg build python36Packages.scipy

@ofborg ofborg bot requested a review from ttuegel January 7, 2021 19:28
@mweinelt mweinelt changed the title openblas: 0.3.10 -> 0.3.13 [staging-20.09] openblas: 0.3.10 -> 0.3.13 Jan 7, 2021
@mweinelt mweinelt added this to Needs review in Staging (stable) Jan 7, 2021
@jonringer
Copy link
Contributor

just saw this, generally we do a cherry-pick, so theres a 1-to-1 correlation between commits in master, to commits in 20.09.

Also, the branches seem to have divereged to the point that it might be worthwhile to just bump the package on 20.09 instead of cherry-picking

@nomeata
Copy link
Contributor Author

nomeata commented Jan 17, 2021

Given that on master there are commits and reversion etc, it seemed cleaner to me to just get the state of the package from master (git checkout master -- …). I just listed the contributing commits for reference, so unless I misunderstand what you are saying, I am just doing what you describe in

it might be worthwhile to just bump the package on 20.09 instead of cherry-picking

am I? (I guess that wasn't well described in the PR description, sorry.)

@twhitehead
Copy link
Contributor

twhitehead commented Jan 28, 2021

Ran into this on our HPC cluster. Would be great if it could get accepted. 😄

@r-burns
Copy link
Contributor

r-burns commented Jan 28, 2021

Well, I think what we would want to do for this backport, is only update the version and hash (and remove the upstreamed patch). The static/shared and split-outputs changes, while beneficial, could potentially cause breakage in package dependees.

@twhitehead
Copy link
Contributor

Sounds reasonable to me. @nomeata, let me know if you are too busy to modify you pull request, and I can submit an alternative one that just bumps the version and drops the obsolete patch.

@jonringer
Copy link
Contributor

Yea, this is kind of an exceptional case, where it was reverted several times. I guess squashing is fine. However, this is only bumping to 0.3.12 not 0.3.13 currently

@twhitehead
Copy link
Contributor

Right. It is just the subject that is wrong. The bump only needs to be to 0.3.12, as per the actual patch and the referenced commits, to fix the scipy issue.

@nomeata nomeata changed the title [staging-20.09] openblas: 0.3.10 -> 0.3.13 [staging-20.09] openblas: 0.3.10 -> 0.3.12 Jan 30, 2021
@nomeata
Copy link
Contributor Author

nomeata commented Jan 30, 2021

Oh, embarrassing. Fixed the title (both PR and commit, which I rebased on latest staging)

Is there more for me to do? I have withdrawn my toes, so if someone else does a proper fix before me and closes this one, they would not be stepped on.

@nomeata nomeata force-pushed the joachim/openblas-0.3.13-20.09 branch from 7d647fd to a8a0f21 Compare January 30, 2021 10:54
@twhitehead
Copy link
Contributor

@r-burns suggested just bumping the version and hash to make the changes as minimal as possible, which seems reasonable to me (not that my opinion matters).

What is you thoughts on this @jonringer? Is this good as is now, or are you thinking it would be best to just bump the version and change the hash?

@jonringer
Copy link
Contributor

@GrahamcOfBorg build python3Packages.numpy

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

I think it's find for now, I would like for it to be updated to 0.3.13 though, as 0.3.12 has a threading issue with high core count cpus OpenMathLib/OpenBLAS#2993 (comment)

Staging (stable) automation moved this from Needs review to Ready Jan 31, 2021
@twhitehead
Copy link
Contributor

Are you wanting 0137860 squashed into this as well, or a separate pull request for it?

@jonringer
Copy link
Contributor

jonringer commented Feb 1, 2021

It would be my preference to have it included and squashed, as it allows me to review the package without failure.

Also would fix runtime issues for people using more than 50 threads on a machine.

this backports the effect of the following commits from `master` to
20.09:

* f52263c treewide: Start to break up static overlay
* d1d536c openblas: 0.3.10 -> 0.3.12
* f715602 Revert "openblas: 0.3.10 -> 0.3.12"
* 840c201 Merge pull request NixOS#101715 from r-ryantm/auto-update/openblas
* e1a59dd openblas: 0.3.10 -> 0.3.12
* 4e29151 Revert "Merge pull request NixOS#101780 from glittershark/bump-openblas"
* 3b4cd4f openblas: 0.3.10 -> 0.3.12
* 692d219 Merge staging-next into staging
* 7902256 openblas: enable multiple outputs
* 92d7b38 openblas: enable on ppc64le
* 0137860 openblas: 0.3.12 -> 0.3.13

The motivation is to unbreak building `python36Packages.scipy` (see
issue 92458)
@nomeata nomeata force-pushed the joachim/openblas-0.3.13-20.09 branch from a8a0f21 to 397c376 Compare February 2, 2021 09:01
@nomeata
Copy link
Contributor Author

nomeata commented Feb 2, 2021

It would be my preference to have it included and squashed, as it allows me to review the package without failure.

Done

@roberth
Copy link
Member

roberth commented Feb 8, 2021

Is this ready for a merge?

@twhitehead
Copy link
Contributor

Is this ready for a merge?

I believe so. @NoMeta has addressed everything that @jonringer has brought up.

@roberth
Copy link
Member

roberth commented Feb 8, 2021

Thanks! Merging into staging-20.09 #112373

@roberth roberth merged commit 4b33a2c into NixOS:staging-20.09 Feb 8, 2021
Staging (stable) automation moved this from Ready to Done Feb 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants