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

fix OpenBLAS 0.3.15 patch to correctly set the CPU core type for Tiger Lake #15845

Conversation

micaeljtoliveira
Copy link
Contributor

(created using eb --new-pr)

@boegel boegel added the bug fix label Aug 3, 2022
@boegel boegel added this to the next release (4.6.1?) milestone Aug 3, 2022
@boegel boegel changed the title Fix OpenBLAS 0.3.15 patch to correctly set the CPU core type for Tiger Lake. fix OpenBLAS 0.3.15 patch to correctly set the CPU core type for Tiger Lake Aug 3, 2022
akesandgren
akesandgren previously approved these changes Aug 31, 2022
Copy link
Contributor

@akesandgren akesandgren left a comment

Choose a reason for hiding this comment

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

LGTM, patch matches changes from 0.3.15 to 0.3.18

@akesandgren
Copy link
Contributor

@boegelbot Please test @ generoso

@boegelbot
Copy link
Collaborator

@akesandgren: Request for testing this PR well received on login1

PR test command 'EB_PR=15845 EB_ARGS= /opt/software/slurm/bin/sbatch --job-name test_PR_15845 --ntasks=4 ~/boegelbot/eb_from_pr_upload_generoso.sh' executed!

  • exit code: 0
  • output:
Submitted batch job 9092

Test results coming soon (I hope)...

- notification for comment with ID 1232778622 processed

Message to humans: this is just bookkeeping information for me,
it is of no use to you (unless you think I have a bug, which I don't).

@boegelbot
Copy link
Collaborator

Test report by @boegelbot
FAILED
Build succeeded for 1 out of 2 (2 easyconfigs in total)
cns2 - Linux Rocky Linux 8.5, x86_64, Intel(R) Xeon(R) CPU E5-2667 v3 @ 3.20GHz (haswell), Python 3.6.8
See https://gist.github.com/89d26aff7ca2dcc5596ba64568a108c0 for a full test report.

@akesandgren
Copy link
Contributor

The updated patch fails to apply to 0.3.12.
Please provide a working, separate, one for 0.3.12

@micaeljtoliveira
Copy link
Contributor Author

@akesandgren Sorry for the long delay in addressing the problem with 0.3.12. I tried to fix the patch, but it seems that for it to actually work one would need to backport a lot more changes from later versions of OpenBLAS. So instead I just kept the current patch as it is for that version. Hope that's okay.

@micaeljtoliveira micaeljtoliveira force-pushed the 20220713201058_new_pr_OpenBLAS0315 branch 2 times, most recently from 394237d to 271fd31 Compare December 22, 2022 03:08
@micaeljtoliveira micaeljtoliveira force-pushed the 20220713201058_new_pr_OpenBLAS0315 branch from 271fd31 to 07f56fe Compare December 22, 2022 03:40
@akesandgren
Copy link
Contributor

Test report by @akesandgren
SUCCESS
Build succeeded for 2 out of 2 (2 easyconfigs in total)
b-an02.hpc2n.umu.se - Linux Ubuntu 20.04, x86_64, Intel(R) Xeon(R) CPU E5-2690 v4 @ 2.60GHz, Python 3.8.10
See https://gist.github.com/482c201aa6e3227e04338df8ec8a4061 for a full test report.

@akesandgren
Copy link
Contributor

@micaeljtoliveira Can you take a look at this, https://gist.github.com/sassy-crick/cc750a24bd1404c1c9a6f8a2c09c10aa ?

Built on a "11th Gen Intel(R) Core(TM) i7-1185G7 @ 3.00GHz"

@easybuilders easybuilders deleted a comment from boegelbot Jan 3, 2023
@micaeljtoliveira
Copy link
Contributor Author

@akesandgren Not quite sure what to do about this. The issue with 0.3.12 is to be expected. As mentioned above, to fix it one would need to backport more fixes made in later versions of OpenBLAS. I tried to do that, but without much success: compilation what still broken even after including several more changes to the patch. Regarding the Lapack timing, that's a strange error, as I can't see any changes in the PR that could do that.

@akesandgren
Copy link
Contributor

Just wanted you to verify that it's what you'd expect for the 0.3.12 build.
The timing error can be ignored, we don't use that part as far as I know.

Copy link
Contributor

@akesandgren akesandgren left a comment

Choose a reason for hiding this comment

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

LGTM

@akesandgren
Copy link
Contributor

Going in, thanks @micaeljtoliveira!

@akesandgren akesandgren merged commit 07b2b0c into easybuilders:develop Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants