Skip to content

windows lapack support#743

Merged
gsilvestrin merged 37 commits intomainfrom
gsilvestrin/windows_lapack
Apr 5, 2023
Merged

windows lapack support#743
gsilvestrin merged 37 commits intomainfrom
gsilvestrin/windows_lapack

Conversation

@gsilvestrin
Copy link
Copy Markdown
Contributor

@gsilvestrin gsilvestrin commented Apr 1, 2023

  • Using vcpkg to locate lapack on windows
  • Fixes and issue handling short path names in Windows, such as C:\Users\Admini~1
  • Refactor rust github actions to remove the duplicated logic installing vcpkg deps


Ok(Self {
inner: Arc::new(LocalFileSystem::new()),
inner: Arc::new(LocalFileSystem::new_with_prefix(expanded_path.deref())?),
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the biggest change - the path provided by the users will be used as prefix by the LocalFileSystem. The prefix need to exist, so I added logic to create the directory if it does not exist yet.

use std::path::Prefix;
use std::path::Prefix::*;

fn get_path_prefix(path: &StdPath) -> Prefix {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Most of the logic here is to get the windows drive letter (C:, D:). There is nothing in this test that is not covered by the other tests, but I'm leaving it to make sure that / and \\ are still working.

shell: powershell
run: |
cd $env:VCPKG_INSTALLATION_ROOT
git pull
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

GHA has a very old version of vcpkg, so some packages were no longer available. By updating the vcpkg sources in each build we solve this issue, but it may increase the build times since we will build openblas / lapack more often.

@gsilvestrin gsilvestrin changed the title Using vcpkg to locate lapack on windows Support for lapack on windows Apr 5, 2023
@gsilvestrin gsilvestrin requested a review from changhiskhan April 5, 2023 00:12
@gsilvestrin gsilvestrin requested a review from eddyxu April 5, 2023 00:12
Comment thread rust/Cargo.toml Outdated

[target.'cfg(target_os = "windows")'.dependencies]
openblas-src = { version = "0.10.8", default-features= false, features = ["system"]}
lapack-src = { version = "0.8", features = ["openblas"] }
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.

This is not os-specific dependency?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We don't need it in the other platforms, and I think you might not need it in windows as well since we are using vcpkg to lookup the packages. I'll try to remove it

Comment thread rust/src/arrow/linalg.rs
type Matrix = Self;
type Sigma = Float32Array;

// FIXME implement svd in windows
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.

yay

@gsilvestrin gsilvestrin linked an issue Apr 5, 2023 that may be closed by this pull request
@gsilvestrin gsilvestrin changed the title Support for lapack on windows windows lapack support Apr 5, 2023
@gsilvestrin gsilvestrin merged commit 249ddcf into main Apr 5, 2023
@gsilvestrin gsilvestrin deleted the gsilvestrin/windows_lapack branch April 5, 2023 17:38
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.

Add Lapack support in windows

2 participants