Change ndarray-linalg backend to intel-mkl

[Tastaturtaste]

This PR addresses EmbarkStudios/cargo-deny#368 by changing the ndarray-linalg backend used for all testing purposes from netlib to intel-mkl. This enables running the tests, and thus development, on Windows, as the intel-mkl is the only backend available on all platforms. This change, however, necessitates restructuring of the tests using this backend as described in the linked issue. Another option would be to cease testing against the older versions of the ndarray-linalg dependency. The second newest release is already over 2 years old and the ndarray version currently tested with that version nearly 3 years old. I would guess most people wouldn't use a new version of argmin with such an old version of ndarray.

I ran all tests locally in Windows and WSL Linux with everything passing. However, on CI, the test solver::gaussnewton::gaussnewton_linesearch::tests::test_next_iter_regression fails due to a change in the result. I currently don't know what the reason for this might be, as the test passes locally on exactly the same version of Ubuntu used in CI. Locally I have an x64 CPU, which should also be the architecture used in CI. The expected result is 2.25 while the actual result in CI is 2.2499999999999964 according to the logs. I don't know if the error message shows all digits. If the message actually shows all digits, this would be a difference of exactly 16 * f64::EPSILON. If you have a clue why this discrepancy between my local run and CI happens, I would be delighted to know.

[stefan-k]

I don't have the ability to do a thorough review right now, but I still want to respond shortly to a few points.

First of all thank you so much for the amount of time and effort and brainpower you have put into this PR and the related issue. Windows support is definitely important, but I can't provide it myself. I highly appreciate it!

Regarding older versions of ndarray: my take on this was to support older versions of ndarray as long as they are not causing problems. I agree that it is not that important to keep supporting them, therefore feel free to drop them.

It will take me another couple of weeks until I am able to review this PR in detail, but I will keep an eye on it and try to respond as well as possible.

[Tastaturtaste]

First of all thank you so much for the amount of time and effort and brainpower you have put into this PR and the related issue. Windows support is definitely important, but I can't provide it myself. I highly appreciate it!

Thanks!

I don't have the ability to do a thorough review right now, but I still want to respond shortly to a few points.

It will take me another couple of weeks until I am able to review this PR in detail, but I will keep an eye on it and try to respond as well as possible.

All good, I know you are currently traveling around, so take your time. Due to personal reasons, I won't be having that much time in the next few weeks anyway.
It would be nice if you could try running the tests, especially the one failing in CI, locally on your machine to see if they fail like in CI or work like locally on my machine.

Regarding older versions of ndarray: my take on this was to support older versions of ndarray as long as they are not causing problems. I agree that it is not that important to keep supporting them, therefore feel free to drop them.

Well, now I implemented the workaround with different packages in a reasonably clean way I think. Of course, it's still a bit hacky and without the workaround it would be cleaner. I don't really care either way, I am completely fine with removing those workaround test packages and dropping the older versions of ndarray.

The failing clippy CI job failure for
cargo clippy -p argmin-math --all-targets --features "primitives,vec,nalgebra_latest-serde,ndarray_latest-serde" -- -D warnings
seems to occur solely due to the useless_vec lint. This already fails on the main branch on my machine. I don't know whats up with that. I didn't want to mess with anything not directly related to windows support, so I left it for now.

The failing job for the cargo-deny for licenses could possibly indicate a bigger problem. The intel-mkl backend is licensed under the "Intel Simplified Software License", which I don't know much about. It certainly is not open-source. This shouldn't really affect the license of argmin itself, as the backend is only there under a feature for testing purposes and users of argmin are expected to depend on a backend themselves explicitly if required. However, a user could potentially enable the features that are only intended for development and thereby depend on intel-mkl through argmin. On the other hand the license of ndarray-linalg which is the immediate dependency of argmin providing the intel-mkl feature, has the standard MIT OR Apache-2.0 license. I don't know how licensing works in these circumstances. A possible alternative to be safe could be to specify ndarray-linalg as a dev-dependency for argmin and remove all features that could enable it as a normal dependency. This would have the drawback that it would always build and link against it while testing, but a user could never depend on it through argmin.

[stefan-k]

Sorry again for the delay!

It would be nice if you could try running the tests, especially the one failing in CI, locally on your machine to see if they fail like in CI or work like locally on my machine.

I did give it a try and ended up with the same error locally. I had a look at the test as well as the code and I moved the assert to the bottom of the test to see if anything else fails too, but that's not the case. I couldn't see anything suspicious, therefore I assume that this is just a numerical inaccuracy based on how MKL does the computations. I'm not that well versed with how errors propagate, but it feels to me that 16 * f64::EPSILON isn't that bad... right? (Just in case its not clear: I am hoping for a bit of confident reassurance from someone with expertise :D)

Well, now I implemented the workaround with different packages in a reasonably clean way I think. Of course, it's still a bit hacky and without the workaround it would be cleaner.

I had a look at the changes and to be honest at first sight I don't fully understand what is going on. I assume you followed "Solution 1" which you described in EmbarkStudios/cargo-deny#368. Did that cause any issues with private implementation details?
The way I understand it, the testing is now done in dedicated crates for each feature/version combination. I'm a bit confused why this is necessary in argmin-math (example for one file):

// The tests expect the name for the crate containing the tested functions to be argmin_math
#[cfg(test)]
use crate as argmin_math;
include!(concat!(
    env!("CARGO_MANIFEST_DIR"),
    "/ndarray-tests-src/eye.rs"
));

I assumed that there a no more tests in argmin-math which relate to the ndarray backend.

I don't really care either way, I am completely fine with removing those workaround test packages and dropping the older versions of ndarray.

I guess your workaround will stay relevant if there are any future versions of ndarray(-linalg) that we may want to support alongside older versions. Therefore I'd leave the decision whether to drop support for older versions or not for later.

The failing clippy CI job failure for cargo clippy -p argmin-math --all-targets --features "primitives,vec,nalgebra_latest-serde,ndarray_latest-serde" -- -D warnings seems to occur solely due to the useless_vec lint. This already fails on the main branch on my machine. I don't know whats up with that. I didn't want to mess with anything not directly related to windows support, so I left it for now.

You can ignore those. Newer versions of Rust come with new/adapted clippy lints, therefore we regularly have new lints popping up in the CI. It's even worse in the CI than locally because the CI uses the beta channel for clippy.

The failing job for the cargo-deny for licenses could possibly indicate a bigger problem. The intel-mkl backend is licensed under the "Intel Simplified Software License", which I don't know much about. It certainly is not open-source. This shouldn't really affect the license of argmin itself, as the backend is only there under a feature for testing purposes and users of argmin are expected to depend on a backend themselves explicitly if required. However, a user could potentially enable the features that are only intended for development and thereby depend on intel-mkl through argmin. On the other hand the license of ndarray-linalg which is the immediate dependency of argmin providing the intel-mkl feature, has the standard MIT OR Apache-2.0 license. I don't know how licensing works in these circumstances. A possible alternative to be safe could be to specify ndarray-linalg as a dev-dependency for argmin and remove all features that could enable it as a normal dependency. This would have the drawback that it would always build and link against it while testing, but a user could never depend on it through argmin.

This is actually my biggest concern. I will have to think about this in detail. Sometimes you just wish you had a legal team!

[Tastaturtaste]

Sorry again for the delay!

No problem, I hope you had a nice time away from the project for a while and now happy holidays!

It would be nice if you could try running the tests, especially the one failing in CI, locally on your machine to see if they fail like in CI or work like locally on my machine.

I did give it a try and ended up with the same error locally. I had a look at the test as well as the code and I moved the assert to the bottom of the test to see if anything else fails too, but that's not the case. I couldn't see anything suspicious, therefore I assume that this is just a numerical inaccuracy based on how MKL does the computations. I'm not that well versed with how errors propagate, but it feels to me that 16 * f64::EPSILON isn't that bad... right? (Just in case its not clear: I am hoping for a bit of confident reassurance from someone with expertise :D)

That's very weird, I just tried again locally and the test still passes locally on my machine. Just to make sure we are running the same test, I use the command
cargo test -p argmin --verbose --all-features solver::gaussnewton::gaussnewton_linesearch::tests::test_next_iter_regression locally on my x86-64-windows-msvc machine as well as in WSL with Ubuntu on the same machine.
Regarding the 16 * f64::EPSILON I think on it's own this error is basically irrelevant. However, in a long chain of numerically very unstable calculations this could of course lead to problems. If anyone does something along the line of x / result_of_optimization, and the result was previously very close to zero then the small delta could potentially result in arbitrarily different results. On the other hand I would argue that someone with a usecase for such a low tolerance for error should probably use error-correction arithmetic anyway.

I had a look at the changes and to be honest at first sight I don't fully understand what is going on. I assume you followed "Solution 1" which you described in EmbarkStudios/cargo-deny#368. Did that cause any issues with private implementation details? The way I understand it, the testing is now done in dedicated crates for each feature/version combination.

Correct, testing is moved to dedicated crates to avoid the problems described in the issue. Namely that using features to select different backend versions doesn't work if those backends specify the same links attribute for different versions. The only way, short of completely ditching all but one versions of the offending backend, is using completely different crates instead of features. Afaik there were no problems related to testing private implementation details, I basically separated all tests as they were into separate files, which are then included in all test crates by macro. I guess the reason is that everything happening in argmin-math is implementation of a public trait of argmin for concrete public types. So everything was already publicly accessible anyway. If there are still questions please don't hesitate to ask.

I'm a bit confused why this is necessary in argmin-math (example for one file):

// The tests expect the name for the crate containing the tested functions to be argmin_math
#[cfg(test)]
use crate as argmin_math;
include!(concat!(
    env!("CARGO_MANIFEST_DIR"),
    "/ndarray-tests-src/eye.rs"
));

I assumed that there a no more tests in argmin-math which relate to the ndarray backend.

The reason I left it in was to indicate to a reader where the tests live. It was a (rather bad) attempt at code as documentation. But it also has the benefit that rust still compiles (and checks!) the tests when changes in the implementation occur. So even if the tests in the implementation files cannot be run because of the missing backend, including them can at least make sure that syntax errors are caught at compile time.

The failing job for the cargo-deny for licenses could possibly indicate a bigger problem. The intel-mkl backend is licensed under the "Intel Simplified Software License", which I don't know much about. It certainly is not open-source. This shouldn't really affect the license of argmin itself, as the backend is only there under a feature for testing purposes and users of argmin are expected to depend on a backend themselves explicitly if required. However, a user could potentially enable the features that are only intended for development and thereby depend on intel-mkl through argmin. On the other hand the license of ndarray-linalg which is the immediate dependency of argmin providing the intel-mkl feature, has the standard MIT OR Apache-2.0 license. I don't know how licensing works in these circumstances. A possible alternative to be safe could be to specify ndarray-linalg as a dev-dependency for argmin and remove all features that could enable it as a normal dependency. This would have the drawback that it would always build and link against it while testing, but a user could never depend on it through argmin.

This is actually my biggest concern. I will have to think about this in detail. Sometimes you just wish you had a legal team!

Yeah, I guess I can't help you with that decision. Personally I would feel safe with the mentioned workaround/alternative. As long as there is no way to pull in the intel-mkl backend directly through argmin itself in any way I don't know how any legislature could twist that into a licensing violation.

[stefan-k]

No problem, I hope you had a nice time away from the project for a while and now happy holidays!

Thanks! :) Belated happy holidays to you too! :)

That's very weird, I just tried again locally and the test still passes locally on my machine. Just to make sure we are running the same test, I use the command cargo test -p argmin --verbose --all-features solver::gaussnewton::gaussnewton_linesearch::tests::test_next_iter_regression locally on my x86-64-windows-msvc machine as well as in WSL with Ubuntu on the same machine.

Yes, I was running this exact test. I don't have MKL installed, therefore I assume that the intel-mkl-src crate downloads it. Could it be that you have a (different) version of MKL installed and intel-mkl-src is linking against that?

Regarding the 16 * f64::EPSILON I think on it's own this error is basically irrelevant. However, in a long chain of numerically very unstable calculations this could of course lead to problems. If anyone does something along the line of x / result_of_optimization, and the result was previously very close to zero then the small delta could potentially result in arbitrarily different results. On the other hand I would argue that someone with a usecase for such a low tolerance for error should probably use error-correction arithmetic anyway.

Sure, these errors can of course lead to increasing errors in the long run -- I simply don't have a feeling of which magnitude of error is to be expected. But I think without knowing what is actually going on in MKL it may be difficult to judge whether this is acceptable or not. Its interesting that we get different results. Either this is due to different MKL versions or due to differences in the processors we are using. Apparently CPUs may also reorder instructions, which could have an effect on the result.

Correct, testing is moved to dedicated crates to avoid the problems described in the issue. Namely that using features to select different backend versions doesn't work if those backends specify the same links attribute for different versions. The only way, short of completely ditching all but one versions of the offending backend, is using completely different crates instead of features. Afaik there were no problems related to testing private implementation details, I basically separated all tests as they were into separate files, which are then included in all test crates by macro. I guess the reason is that everything happening in argmin-math is implementation of a public trait of argmin for concrete public types. So everything was already publicly accessible anyway. If there are still questions please don't hesitate to ask.

This sounds great! I did not expect any issues with private implementation details, but if there were, it would hint to an issue (because as you said, its all public interfaces).

The reason I left it in was to indicate to a reader where the tests live. It was a (rather bad) attempt at code as documentation. But it also has the benefit that rust still compiles (and checks!) the tests when changes in the implementation occur. So even if the tests in the implementation files cannot be run because of the missing backend, including them can at least make sure that syntax errors are caught at compile time.

Ah, this also a good idea. I would appreciate it though if you could add a short comment to indicate that, because I am for sure going to forget that when I look at the code the next time ;)

I don't know how licensing works in these circumstances. A possible alternative to be safe could be to specify ndarray-linalg as a dev-dependency for argmin and remove all features that could enable it as a normal dependency. This would have the drawback that it would always build and link against it while testing, but a user could never depend on it through argmin.

[...]

Yeah, I guess I can't help you with that decision. Personally I would feel safe with the mentioned workaround/alternative. As long as there is no way to pull in the intel-mkl backend directly through argmin itself in any way I don't know how any legislature could twist that into a licensing violation.

Just for my understanding, when you talk about argmin in this paragraph, you mean argmin-math, right?
I just had a thought: Using ndarray-linalg with any backend at all is only required for the tests. As the tests are not published anyway (and users can't depend on them), there should be no issue with the MKL licensing, right? I may have a misunderstanding though.
But nevertheless, after thinking about it, I think you are right with your proposed approach.

[Tastaturtaste]

So, I just added a comment for the explanation for why the tests are still included in the argmin-math ndarray source and now the previously failing test passes in CI in my fork? Really weird.

Could it be that you have a (different) version of MKL installed and intel-mkl-src is linking against that?

I don't think so, intel-mkl is linked statically, I wouldn't think I have a static lib already preinstalled in wsl. But I also don't really how to check which version is used in the test besides looking at the compilation logs and comparing to my local build output. And there everything is the same.

Apparently CPUs may also reorder instructions, which could have an effect on the result.

While CPUs can reorder instructions, they don't (or at least shouldn't in the absence of bugs) do that if results could change. Afaik CPUs also don't have something like a fast-math mode where they ignore floating point peculiarities. And compilation with a fast-math flag shouldn't make the binary non-deterministic I think. What could instead possibly influence the test are differing gcc versions.

Ah, this also a good idea. I would appreciate it though if you could add a short comment to indicate that, because I am for sure going to forget that when I look at the code the next time ;)

As mentioned I added a comment above all the test inclusions in the argmin-math ndarray source. However, I have to revise my previous statement regarding the building and compilation a bit. The way it works now is that all tests can be run directly in the argmin-math crate as long as no feature for ndarray-linalg is enabled. As soon as such a feature is enabled, an error for a missing dynamic library will occur when compiling. Since ndarray-linalg is only necessary for the ArgminInv trait, most code can still be tested in argmin-math itself, with the only exception being the tests in [inv.rs](https://github.com/Tastaturtaste/argmin/blob/win-netlib-fix/argmin-math/src/ndarray_m/inv.rs).

Just for my understanding, when you talk about argmin in this paragraph, you mean argmin-math, right?

I mean both, because I had the same thought as you:

I just had a thought: Using ndarray-linalg with any backend at all is only required for the tests. As the tests are not published anyway (and users can't depend on them), there should be no issue with the MKL licensing, right? I may have a misunderstanding though.

As long as the intel-mkl dependency cannot be pulled into the regular dependency graph through a feature there should be no licensing problem. Since the argmin-math tests that need a backend are now run in separate crates, the argmin-math crate itself doesn't have any feature to pull in a backend anymore. So the testing in a separate crate helps here, too, even if it would have been possible to use features to select the backend for testing as was done previously. I really think with this approach the licensing problem isn't actually a problem anymore. To make that explicit and prevent accidentally publishing the tests I set publish = false in their Cargo.toml. I also changed licenses.private.ignore = true in deny.toml so cargo-deny does not check licenses of unpublished crates.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (355a4a2) 90.16% compared to head (45e56db) 90.16%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #369   +/-   ##
=======================================
  Coverage   90.16%   90.16%           
=======================================
  Files         126      141   +15     
  Lines       19679    19679           
=======================================
  Hits        17744    17744           
  Misses       1935     1935           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[stefan-k]

So, I just added a comment for the explanation for why the tests are still included in the argmin-math ndarray source and now the previously failing test passes in CI in my fork? Really weird.

Ha, fun! :D It still fails on my local machine. I updated Rust from 1.74.1 to 1.75, ran cargo update and cargo clean, but still the same failure. The CI passes as well. Guess I'll just wait until it works for me as well ;)
Even clippy stopped complaining, even though you didn't rebase the commit yet (the clippy fixes are already in main).

While CPUs can reorder instructions, they don't (or at least shouldn't in the absence of bugs) do that if results could change. Afaik CPUs also don't have something like a fast-math mode where they ignore floating point peculiarities. And compilation with a fast-math flag shouldn't make the binary non-deterministic I think. What could instead possibly influence the test are differing gcc versions.

Since MKL is apparently multi-core I assumed that the order in which operations are performed isn't necessarily fixed, but apparently that's not true. Even if that was the case, the test uses matrices/vectors so small that I'm fairly sure all operations run on a single core.

As mentioned I added a comment above all the test inclusions in the argmin-math ndarray source. However, I have to revise my previous statement regarding the building and compilation a bit. The way it works now is that all tests can be run directly in the argmin-math crate as long as no feature for ndarray-linalg is enabled. As soon as such a feature is enabled, an error for a missing dynamic library will occur when compiling. Since ndarray-linalg is only necessary for the ArgminInv trait, most code can still be tested in argmin-math itself, with the only exception being the tests in [inv.rs](https://github.com/Tastaturtaste/argmin/blob/win-netlib-fix/argmin-math/src/ndarray_m/inv.rs).

Ah yes, that makes sense.

[...] I really think with this approach the licensing problem isn't actually a problem anymore.

Yes, I agree!

To make that explicit and prevent accidentally publishing the tests I set publish = false in their Cargo.toml. I also changed licenses.private.ignore = true in deny.toml so cargo-deny does not check licenses of unpublished crates.

Another great idea, thanks!

However, now cargo deny complains about a couple of licenses because MKL is also used in argmin. Since it's not part of a feature it should -- as you said -- not be possible to activate this when using argmin as a dependency, therefore that's not an issue. We just need to allow the licenses (ISC, OpenSSL, and the MKL one, although I'm unsure how this should be done because intel-mkl-src only provides a license file).
In the long run (after this is merged), I think the tests which need ndarray-linalg should probably be rewritten to nalgebra and the examples should be dedicated crates (#307). Then this shouldn't be an issue anymore.

[Tastaturtaste]

I was just playing around with cargo tree and cargo deny to see if if I could teach the tools to exclude dev-dependencies automatically. While doing so I had some interesting findings:

1. cargo tree -i intel-mkl-src --all-features -e "no-dev,features" clearly shows intel-mkl-src is still a dependency of argmin-math
   • This is the case because the workspace still uses resolver = "1" implicitly, which unifies features of normal dependencies and dev-dependencies
   • Since ndarray-linalg has the intel-mkl-static feature enabled as a dev-dependency, the normal dependency has it enabled as well.
2. Changing the resolver by adding resolver = "2" in the workspace Cargo.toml prevents feature unification of normal and dev-dependencies
   • cargo tree -i intel-mkl-src --all-features -e "no-dev,features" now shows no dependency on intel-mkl-src
3. cargo deny seems to still use resolver = "1" (or another custom solution), because
   • even after changing the workspace to resolver = "2" cargo deny --log-level error --all-features --exclude-dev check bans licenses sources still checks and rejects intel-mkl-src
4. The resolver is a global option set in the root-crate, so dependents of argmin are not influenced by the resolver version used in the workspace
5. Since the intel-mkl-src feature is enabled in a dev-dependency of argmin only, I think actual dependents should never see intel-mkl-src in their dependency graph as we already concluded before. But it would be nice if the tools also knew that without requiring a blanko-allow for the unwanted licenses.

I don't know yet what to do with this information, but I wanted to document it here just in case. Because of this dependency issue, but also because the warning

warning: virtual workspace defaulting to resolver = "1" despite one or more workspace members being on edition 2021 which implies resolver = "2"

annoys me everytime I compile, would you be opposed to changing the resolver to resolver = "2"?

[stefan-k]

Thanks yet again for the amount of effort you put into this PR, I really appreciate it! I don't have much time right now to respond in detail, but I wanted to quickly reply regarding that point:

I don't know yet what to do with this information, but I wanted to document it here just in case. Because of this dependency issue, but also because the warning

warning: virtual workspace defaulting to resolver = "1" despite one or more workspace members being on edition 2021 which implies resolver = "2"

annoys me everytime I compile, would you be opposed to changing the resolver to resolver = "2"?

This is already done in main, therefore if you rebase your PR you should have this change. :)

[Tastaturtaste]

Thanks, I appreciate your appreciation :)

I just opened an an issue for the differences in resolved dependencies between cargo tree and cargo deny. Maybe someone there knows an incantation to avoid that.

[Tastaturtaste]

Thanks, I appreciate your appreciation :)

I just opened an an issue for the differences in resolved dependencies between cargo tree and cargo deny. Maybe someone there knows an incantation to avoid that.

They already fixed krates, which was the responsible dependency for cargo-denys issue. I just did a fresh install of cargo-deny (without --locked) and tried cargo deny --log-level error --all-features --exclude-dev check bans licenses sources successfully. No license issues detected. I imagine the CI version is not up to date yet with this version.

I also rebased again on the latest main and resolved some conflicts that popped up.

[stefan-k]

Thanks again, this looks good! There is nothing to complain, but I'm still not sure how to proceed because of cargo-deny. Is it possible to temporarily allow the license? We just have to make sure that we don't forget to disable it once a new cargo-deny version is out.

[Tastaturtaste]

I can put the license on the whitelist if you want, but tracking when to take it off of the list again would be manual. It would probably be a good idea to have another PR lined up to take it off the whitelist again from the get-go so it doesn't get missed. If you want I can do that.

[stefan-k]

This sounds like a reasonable approach!

[stefan-k]

Thanks a lot!

[Tastaturtaste]

Thanks a lot!

No problem, it was fun to work on this!
Thanks for your work on maintaining this crate.