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

include mkl libraries for intel on linux and MacOS #81

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

iulusoy
Copy link

@iulusoy iulusoy commented Mar 29, 2024

  • mkl libraries included for most MacOS versions
  • mkl libraries included for all linux versions
  • new test for mkl installation
  • currently is not the default, the mkl test is run conditionally. With the inclusion of math libraries for more compilers+OS, the tests should run as default

Copy link
Contributor

@wpbonelli wpbonelli left a comment

Choose a reason for hiding this comment

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

to get ci tests to run you can add the math branch to the pull_request trigger in test.yml

- name: Test MKL libraries
# we could also exclude 2021.5 for all macos versions here
# this needs refactoring at some point - should the install
# fail if no mkl version exists?
Copy link
Contributor

@wpbonelli wpbonelli Apr 16, 2024

Choose a reason for hiding this comment

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

I think it makes sense to fail, given we aim to support all intel versions and runner images barring a few exceptions. In general I think we should fail if we can't satisfy the user's selected configuration.

On the other hand, we don't fail with compiler: intel on mac, we just fall back to intel-classic. Maybe there is an argument for changing this.

There is also the question of what to do if the user requests MKL with a non-intel compiler. I would say fail in that case too?

@wpbonelli
Copy link
Contributor

@iulusoy I rebased math from main, could you rebase this or enable contributions? I think we could merge soon and keep iterating

@iulusoy
Copy link
Author

iulusoy commented Apr 17, 2024

@iulusoy I rebased math from main, could you rebase this or enable contributions? I think we could merge soon and keep iterating

Did so and also added you to the repo.

@wpbonelli
Copy link
Contributor

thanks @iulusoy I joined. in future you can just check this box to give a repo maintainer access to a PR branch from your fork without making them a full contributor

allow_edits

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@wpbonelli
Copy link
Contributor

@iulusoy can you revert the CSV changes and/or rebase? I updated math from main.

In general you can ignore the auto-update PRs on your fork (really they should just run on the main repo, I'll change that at some point) and just rebase from upstream instead.

@wpbonelli
Copy link
Contributor

@iulusoy any idea on the test failures?

@iulusoy
Copy link
Author

iulusoy commented Jun 28, 2024

@iulusoy any idea on the test failures?

No, not really - I need to try this out in a container to see what is happening. Have it on my to-do list.

What about extending the math support to other compilers/windows? Should we assign responsibilities to move this forward?
I will have some more time for this towards the end of next month, so I could look at gnu compiler; but I have no experience with installing them on windows and would like to pass that to someone who is more experienced with it.

@wpbonelli
Copy link
Contributor

What about extending the math support to other compilers/windows? Should we assign responsibilities to move this forward?
I will have some more time for this towards the end of next month, so I could look at gnu compiler; but I have no experience with installing them on windows and would like to pass that to someone who is more experienced with it.

I'm still not sure on the approach for other toolchains. Since BLAS/LAPACK were added to stdlib an alternative besides those mentioned in #55 could be an install_stdlib option, which could install the reference implementation, and install_mkl could be a one-off specific to the Intel toolchain.

This PR could be merged with mac/linux support and windows could come in a separate PR, then math could merge into main and we could release this as an Intel-specific feature.

Just thinking out loud

@iulusoy
Copy link
Author

iulusoy commented Aug 23, 2024

@iulusoy any idea on the test failures?

Seems that the apt installation of the mkl libraries changes the setvars script for mkl version 2021.1. Because of this I changed the order of the compiler vs mkl installation. The PR will be updated shortly.

@iulusoy
Copy link
Author

iulusoy commented Sep 3, 2024

@wpbonelli do you think this can be merged now, or do we need some more tests?

@wpbonelli
Copy link
Contributor

@iulusoy do you mind doing a final rebase? looks like the diff still has changes in the compat scripts and CSVs.

@iulusoy
Copy link
Author

iulusoy commented Sep 6, 2024

@iulusoy do you mind doing a final rebase? looks like the diff still has changes in the compat scripts and CSVs.

Should be no more diffs to main now, other than the actual changes.

@wpbonelli
Copy link
Contributor

Looks like this is still behind. Maybe helpful to squash everything down to your first commit e.g. git reset --soft 56f36a1, git commit --amend --no-edit before doing an interactive rebase so you only have to resolve conflicts once.

Copy link
Contributor

@wpbonelli wpbonelli left a comment

Choose a reason for hiding this comment

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

looks good to me, merging now.

@wpbonelli wpbonelli merged commit c4c7dba into fortran-lang:math Sep 20, 2024
176 checks passed
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.

2 participants