gonum / blas

A BLAS implementation for Go [DEPRECATED]
172 stars 16 forks source link

blas: add stability promise #171

Closed kortschak closed 5 years ago

kortschak commented 8 years ago

I think we can add this now.

The only concern I have is that I still think I want to add column major/row major conversion types and methods. I can't see that this will break this promise.

Also, are there any security perspective that we need to consider?

@gonum/developers Please take a look.

jonlawlor commented 8 years ago

I'd like to suggest adding something about what versions of go we're willing to support as well. Something like:

We will support at least the most recent xxx stable releases of go.

btracey commented 8 years ago

It may be worthwhile to have an "extra notes" section

Extra notes

The second point seems important to me, not just in BLAS, but more generally in gonum. Some wordsmithing is needed.

I assume there are security points we could need to consider. We are using cgo/unsafe.

btracey commented 8 years ago

Two other comments: We should explicitly tag a version of this 1.0. I would say we add a note to the top of this at the moment saying it is not yet in effect.

Why wait? It seemed like we wanted gonum.org/gonum/blas/blas64

kortschak commented 8 years ago

I'd like to suggest adding something about what versions of go we're willing to support as well. Something like:

We will support at least the most recent xxx stable releases of go.

I don't think this is the place for that. I would think README.md is the place.

  • The BLAS API does not promise behavior with NaN. Results may change if input data has a NaN value

This should be in package level docs rather than the STABILITY document. Once it's in the package level docs, the stability statement covers it.

  • We reserve the right to modify the order of floating point operations. That is, future results are not guaranteed to have the same binary representation, just be accurate to within floating point precision.

I don't think this needs to be said. There is no equivalent in the Go1 promise and math floating point implementations have changed over the years.

I assume there are security points we could need to consider. We are using cgo/unsafe.

This is a risk for users (though one that can be mitigated by using the appengine tag), but the risk needs to be assessed under a reasonable threat model. I don't see one that involves a fix that would break the promise which hold only on the API. If you can think of one, or you think that one could have a reasonable chance of being found, we can add the security clause.

Two other comments: We should explicitly tag a version of this 1.0. I would say we add a note to the top of this at the moment saying it is not yet in effect.

It should be tagged. The best approach to holding off is to hold off - this PR won't go stale, so there is no rush to merge it if we are not ready.

Why wait? It seemed like we wanted gonum.org/gonum/blas/blas64

(Presumably gonum.org/blas/blas64?)

This should happen soon. We're already seeing appreciable uptake and the longer we wait, the more code we break when we do migrate import paths.

I suggest a road map for this:

  1. Set up the gonum.org redirection (gonum.org probably wants a front page too).
  2. Migrate all the imports within gonum (care taken with internal).
  3. Announce the migration widely.
  4. After a reasonably wait (1 month?), impose strict import paths.
btracey commented 8 years ago

I assume there are security points we could need to consider. We are using cgo/unsafe.

This is a risk for users (though one that can be mitigated by using the appengine tag), but the risk needs to be assessed under a reasonable threat model. I don't see one that involves a fix that would break the promise which hold only on the API. If you can think of one, or you think that one could have a reasonable chance of being found, we can add the security clause.

I guess I'm always wary. I don't see anything that would need to change the API, but I also don't very much about security exploits and what things could have to change. It seems like a logical clause to include.

Two other points before we fix forever

The best approach to holding off is to hold off

Duh. Thanks.

This should happen soon. We're already seeing appreciable uptake and the longer we wait, the more code we break when we do migrate import paths.

Agreed. After this week I should have more time.

I suggest a road map for this:

  • Set up the gonum.org redirection (gonum.org probably wants a front page too).
  • Migrate all the imports within gonum (care taken with internal).
  • Announce the migration widely.
  • After a reasonably wait (1 month?), impose strict import paths.

This path sounds good.

I know approximately zero about websites. We should probably start a gonum-dev thread on that? I'm sure the redirect is easy, but other decisions like hosting, layout, and content could be made collectively. In particular, hosting the blog at gonum.org rather than merely gist may allow us runnable code examples using the gonum tools.

kortschak commented 8 years ago

I guess I'm always wary. I don't see anything that would need to change the API, but I also don't very much about security exploits and what things could have to change. It seems like a logical clause to include.

It might be worth asking on golang-nuts. I honestly can't see a reasonable threat model, but I'm sure code review of numerous exploit providing changes have similar comments.

Two other points before we fix forever

  • Do we promise anything about stability on panic messages? I'd like the flexibility to change them if we need to. As we've found, the Go1 contract still isn't particularly clear on how much strings are not allowed to change. Changing panic messages changes recover behavior.

No. The panics that are meaningful to recover from we already provide a mechanism for. All other panics are our fault and should be fixed.

  • How do you feel about changing blas/cgo to bcgo and lapack/cgo to lcgo. It's really nice to be able to interface with goimports, and the current overlap makes it trickier than I'd like. It only happens at the main function, so it's not that huge of a deal, but still.

Import renaming covers that when it happens, unless you can find a more aesthetically pleasing pair of names.

I know approximately zero about websites. We should probably start a gonum-dev thread on that? I'm sure the redirect is easy, but other decisions like hosting, layout, and content could be made collectively. In particular, hosting the blog at gonum.org rather than merely gist may allow us runnable code examples using the gonum tools.

SGTM for discussion.

Further on the float operation ordering, maybe add that in the package doc for matrix. I'll leave that up to you. I don't think it's worth it, but I could be wrong.

kortschak commented 8 years ago

Float ordering is covered by the first exception:

  • Unspecified behavior. Programs that depend on unspecified behavior may break in future releases.
jonlawlor commented 8 years ago

I think the panic messages should remain stable. I understand the use of panic instead of the more idiomatic result, err, but allowing the panic messages to change means there is very little users can rely on for exceptional behavior.

Also, regarding NaN behavior: are the tests that use NaN going to be made moot? Can a behavior be verifiable if it is undefined?

kortschak commented 8 years ago

I think the panic messages should remain stable. I understand the use of panic instead of the more idiomatic result, err, but allowing the panic messages to change means there is very little users can rely on for exceptional behavior.

For the panics that are not internal (i.e. those that panic matrix.Error type) they are already covered by the promise since the matrix.Error values are exported. Internal panics, those that are strings, it is not the job of the caller to do anything except report an issue or log the panic since they are definitively a bug.

Also, regarding NaN behavior: are the tests that use NaN going to be made moot? Can a behavior be verifiable if it is undefined?

Unspecified.

kortschak commented 8 years ago

Security clause added.

kortschak commented 8 years ago

@jonlawlor How do we deal with travis/coveralls with vanity imports? I guess the blunt way is to mv to the correct location, but there must be a more elegant approach.

kortschak commented 8 years ago

And there answer is easily found: https://docs.travis-ci.com/user/languages/go#Go-Import-Path

go_import_path: gonum.org/blas
kortschak commented 8 years ago

Import renaming covers that when it happens, unless you can find a more aesthetically pleasing pair of names.

I'd be OK with "C_lapack" and "C_blas".

btracey commented 8 years ago

I'd be OK with "C_lapack" and "C_blas"

SGTM

kortschak commented 8 years ago

Does that have implications for the natives?

jonlawlor commented 8 years ago

I know approximately zero about websites. We should probably start a gonum-dev thread on that? I'm sure the redirect is easy, but other decisions like hosting, layout, and content could be made collectively. In particular, hosting the blog at gonum.org rather than merely gist may allow us runnable code examples using the gonum tools.

I'm also a novice involving websites, but I've done a redirect and github.io blog, so that part is pretty easy. I am unsure how to incorporate runnable examples, it would take some adaptation of the playground code, and there are obvious security issues that I am not sure how to resolve. Plus some cloud compute time probably. It would be really great to have runnable examples, but I don't think anything would preclude having a static website and adding examples later (when someone who actually knows what they are doing there is available).

btracey commented 8 years ago

Does that have implications for the natives?

Possibly, although I've never needed to import both natives. The packages are registered by default, and otherwise everything I've wanted to use is behind blas64, so in normal code I've never actually imported native. I've only imported native during the development of BLAS (even lapack doesn't import it directly). Basically, the name clash doesn't seem like an inconvenience with native as it is with cgo.

kortschak commented 8 years ago

I was thinking about this yesterday and two options seemed worth considering

  1. gonum_blas and gonum_lapack
  2. moving the Go implementations into gonum/blas and gonum/lapack.

I'm not convinced by either of them (I prefer 2.), but the lack of symmetry with cgo's renaming troubles me.

vladimir-ch commented 5 years ago

Closing because this repository is no longer maintained. Development has moved to https://github.com/gonum/gonum.