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.

Sign up for GitHub

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

Jump to bottom

Update package to use jll #204

Merged
merged 3 commits into from
Jun 9, 2020
Merged

Update package to use jll #204

merged 3 commits into from
Jun 9, 2020

Conversation

aviks
Copy link
Contributor

@aviks aviks commented Jun 4, 2020 •
edited
Loading

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

@aviks
Update package to use jll
4069986 
Minimum Julia version is 1.3
@aviks
Copy link
Contributor Author

aviks commented Jun 4, 2020

Failure on nightly is apparently due to IRTools not supporting julia nightly

WARNING: could not import Compiler.just_construct_ssa into Wrap
ERROR: LoadError: LoadError: InitError: UndefVarError: Params not defined
Stacktrace:
 [1] top-level scope at none:1
 [2] eval at .\boot.jl:331 [inlined]
 [3] define_typeinf_code2() at C:\Users\appveyor\.julia\packages\IRTools\BpoqK\src\reflection\reflection.jl:21
 [4] __init__() at C:\Users\appveyor\.julia\packages\IRTools\BpoqK\src\IRTools.jl:37

@DhairyaLGandhi
Copy link
Member

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

@ViralBShah
Copy link
Member

I thought NNPACK was always used when I use Flux.

@DhairyaLGandhi
Copy link
Member

That was when we had to transition. We'd made the Julia backend the default at the time

@aviks aviks mentioned this pull request Jun 4, 2020
Closed
giordano
giordano reviewed Jun 4, 2020
View reviewed changes
Project.toml
@@ -1,18 +1,18 @@
name = "NNlib"
uuid = "872c559c-99b0-510c-b3b7-b6c96a88d5cd"
version = "0.6.6"
version = "0.6.7"
Copy link

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

Copy link
Member

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

Copy link
Member

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.

Copy link
Contributor Author

@aviks aviks Jun 9, 2020 •
edited
Loading

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.

Copy link

Choose a reason for hiding this comment

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

Quoting from https://github.com/JuliaRegistries/RegistryCI.jl/blob/729889777565fb0275110034ee933bd509b8962e/src/AutoMerge/guidelines.jl#L94-L100

A patch release is not allowed to narrow the supported ranges of Julia versions

Copy link

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

Copy link
Member

Choose a reason for hiding this comment

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

Quoting from https://github.com/JuliaRegistries/RegistryCI.jl/blob/729889777565fb0275110034ee933bd509b8962e/src/AutoMerge/guidelines.jl#L94-L100

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

Copy link

Choose a reason for hiding this comment

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

Uhm, right 😅

CarloLucibello
CarloLucibello reviewed Jun 6, 2020
View reviewed changes
.travis.yml Outdated Show resolved Hide resolved
CarloLucibello
CarloLucibello reviewed Jun 6, 2020
View reviewed changes
src/NNlib.jl Outdated Show resolved Hide resolved
CarloLucibello
CarloLucibello reviewed Jun 6, 2020
View reviewed changes
src/NNlib.jl
@@ -1,14 +1,19 @@
module NNlib
using Pkg
Copy link
Member

Choose a reason for hiding this comment

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

do we need this?

Copy link
Contributor Author

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

aviks and others added 2 commits June 6, 2020 12:48
@aviks @CarloLucibello
Update src/NNlib.jl
d0e9775 
Co-authored-by: Carlo Lucibello <carlo.lucibello@gmail.com>
@aviks @CarloLucibello
Update .travis.yml
dbb1d7b 
Co-authored-by: Carlo Lucibello <carlo.lucibello@gmail.com>
@CarloLucibello
Copy link
Member

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?

@aviks
Copy link
Contributor Author

aviks commented Jun 9, 2020

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.

@CarloLucibello
Copy link
Member

ok, I'll merge, we can sort out the version number later

@CarloLucibello CarloLucibello merged commit 0d16973 into FluxML:master Jun 9, 2020
@CarloLucibello CarloLucibello mentioned this pull request Jun 15, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giordano giordano giordano left review comments

@CarloLucibello CarloLucibello CarloLucibello left review comments

@DhairyaLGandhi DhairyaLGandhi DhairyaLGandhi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Provide artifact for BB download
5 participants
@aviks @DhairyaLGandhi @ViralBShah @CarloLucibello @giordano