-
-
Notifications
You must be signed in to change notification settings - Fork 122
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
Update package to use jll #204
Conversation
Minimum Julia version is 1.3
Failure on nightly is apparently due to IRTools not supporting julia nightly
|
Yes, nightly needs a new tag for IRTools Generally looks good. We had noticed issues earlier when making nnpack the default, so will run the Flux test suite on this |
I thought NNPACK was always used when I use Flux. |
That was when we had to transition. We'd made the Julia backend the default at the time |
@@ -1,18 +1,18 @@ | |||
name = "NNlib" | |||
uuid = "872c559c-99b0-510c-b3b7-b6c96a88d5cd" | |||
version = "0.6.6" | |||
version = "0.6.7" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a restriction of allowed Julia versions warrants a minor version bump
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine, julia users on any version won't see any breaking change. Propagating to the ecosystem a minor version update is slightly annoying and I am always happy to avoid it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest a version bump here too if it changes underlying behaviour of a package. In this case the pr makes use of nnpack by default, so should be reflected in versioning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
propagating to the ecosystem a minor version update is slightly annoying
That was my thought as well.
In this case the pr makes use of nnpack by default
I'm not sure I understand how that worked. I thought the current code is functionally equivalent -- if the binaries are present, they'll be used. Just that binaries are now available for more platforms. But maybe I'm misunderstanding something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A patch release is not allowed to narrow the supported ranges of Julia versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Propagating to the ecosystem a minor version update is slightly annoying and I am always happy to avoid it
This is the problem of being in 0.x for too long. Narrowing the supported ranges of Julia versions always requires a minor version bump, but in 1+ series this is not a big deal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A patch release is not allowed to narrow the supported ranges of Julia versions
this applies only to if (old_version >= v"1") || (new_version >= v"1")
though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhm, right 😅
@@ -1,14 +1,19 @@ | |||
module NNlib | |||
using Pkg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for the error message to say what platform you are on. https://github.com/FluxML/NNlib.jl/pull/204/files#diff-04ed3dd551723d9193b5371386dea323R14
Co-authored-by: Carlo Lucibello <carlo.lucibello@gmail.com>
Co-authored-by: Carlo Lucibello <carlo.lucibello@gmail.com>
I tried this PR in combination with Flux and Flux's test works fine. The NNPACK version seems quite outdated, it would be nice to have a bump for that, where can I ping for it? |
The build recipe is here: https://github.com/JuliaPackaging/Yggdrasil/blob/master/N/NNPACK/build_tarballs.jl , it needs to be updated to the latest tags. There seem to be quite a few dependencies, so might be a bit of work. |
ok, I'll merge, we can sort out the version number later |
Minimum Julia version is now 1.3
Build script
(As an aside, this package seems to check in the Manifest.toml. Is a good idea?)
Fix #192