-
-
Notifications
You must be signed in to change notification settings - Fork 268
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
[WIP/RFC] Conditional dependencies and package features #977
Conversation
Or maybe just Pkg.@feature "cuda" include("cuda.jl") |
Thanks for the PR, @Roger-luo. We definitely needs something like this but I need to think a bit more about the problem. Just wanted to let you know so that you know this isn't being ignored. |
@StefanKarpinski Thanks for replying. I'd love to discuss this problem with you if you want, or maybe I could join Pkg's online meeting at some point. We had a painful time while trying to release a package with gpu support. I'm actually working a little bit to make this design work tho. lol. |
Is this still being considered for merging? This is exactly what we need for Oceananigans.jl as it supports running on CPUs and GPUs but we don't want to add/build CUDA packages if no GPU is available. It's fine when a user without a GPU adds the package as they just see a bunch of CUDA warnings, but running on mybinder.org/Kubernetes it seems to interpret something as a non-zero return code and it fails to run. Thank you for working on this @Roger-luo! |
@ali-ramadhan thanks, but there's nothing here in this PR for now actually. I was mainly open this to get comments and review on the proposal and once the proposal is decided then people could implement it. @StefanKarpinski says we need some more discussion on this, but I'm not sure what the exactly timeframe is. I'm willing to join the discussion if there's one about it. I guess people are busy with some other on-going work at the moment, maybe we could discuss this during JuliaCon, etc. PS. I think a lot people need this feature ASAP |
I agree with @Roger-luo that certain parts of the ecosystem really, really need something like this (especially the GPU-powered part), but of course one should never rush something like this if proposed solutions are premature. It would be great to lay out some other ideas for how to implement this functionality (even the not-so-great ones), as well as the upsides and downsides of each approach, so we can discuss this in the open and get some more input from the community. |
@jpsamaroo yes, as the title indicates, I'm looking for comments and other potential solutions. My proposal is mainly copied from rust cargo with limited edition myself, so it might not suits Julia very well in some cases. But I haven't see any other comments on what my proposal itself here... Maybe people just feel this looks good? |
Thank you @Roger-luo, for implementing this. I have a few comments/questions:
|
@tkf thanks for commenting!
I'm using this in
What about let each environment contain a set of optional features? which will be enabled by default, it is global for packages inside the environment. I discussed the global sharing issue with @StefanKarpinski , there is a problem is how these feature constant flag propagates by the dependency we haven't make clear at the moment.
The idea to have this macro is basically just because there's
No, I think the behaviour of each environment should be explicit, unless you type which feature to enable explicitly by This is useful because you can have
Yes, if the feature can be enabled explicitly, then this will just work.
do you mean I could just write add Flux[cuda, tpu] for multiple features? yeah, this looks better than mine. I thought it was ugly as well. Is this compatible with shell scripts btw? since I also hope our package mode is consistent with shell, so we could have CLIs in the future. |
Wow, this is a super clever hack! BTW, an alternative strategy could be something like const BatchedRoutinesCUDA = try
Base.require(Base.PkgId(Base.UUID(0x00000000000000000000000000000000),
"BatchedRoutinesCUDA"))
catch
nothing
end
if BatchedRoutinesCUDA !== nothing
using .BatchedRoutinesCUDA.CuArrays
using .BatchedRoutinesCUDA.GPUArrays
end in (say)
I think we are probably speaking the same thing. I wasn't clear enough. What I was thinking was to add [[Flux]]
deps = ["..."]
git-tree-sha1 = "08212989c2856f95f90709ea5fd824bd27b34514"
uuid = "587475ba-b771-5e3f-ad9e-33799f191a9c"
version = "0.8.3"
features = ["cuda"] Then Base's package loader would take
Yes, also something like
FYI, there's already |
Yeah, it seems an alternative indeed. But I currently put them to separate packages now, where
Yeah, I tried But yeah |
@Roger-luo Given that JuliaLang/julia#37595 is merged and we (will) have https://github.com/JuliaPackaging/Preferences.jl, I think we can close it now? IIUC, combining conditional dependencies #1285 and Preferences.jl will cover your use-cases. |
I guess we can close this now that we have package extensions |
Backgrond
Conditional dependency is useful when a package provides optional enhancement based on other packages, e.g Flux.jl allows to use CuArray to accelerate the calculation with CUDA. And in the future, there may be package depends on MKL.jl (MKL has some non-BLAS/LAPACK routines, like
batched_gemm
).However, current solution, the Requires package, can not provide explicit dependency in
Project.toml
, and it does not support multiple package at the moment: JuliaPackaging/Requires.jl#54 thus I believe this can just be a temporary work around.And it is not convenient and does not make sense to enable the feature by explicitly using several package:
A package
Foo
depends onCUDAnative
andCuArrays
when cuda is available, but neither of the following make senseWe found this is very in-convenient while developing CuYao.jl and BatchedRoutines.jl, since part of the implementation is implemented with
CUDAnative
/GPUArray
/CuArray
, but actually just requires to enable thecuda
feature.This is because
Foo
may wrap types inCuArrays
/CUDAnative
, or it will just useCuArray
. And what we actually want to do here, is just to precompile and build the CUDA related part of Foo when the feature CUDA is enabled.Proposal
The proposal here is inspired by rust-cargo. It will add a new field
[features]
inProject.toml
and for each package inManifest.toml
, and also forPackageSpec
type.TOML changes
Since there is a new field for each package, in order to allow developer use a package with specific feature, more detailed information about dependencies in
Project.toml
will be allowedhere
will be equivalent to
CLI changes
And a new key word argument for
add
How to know which feature is enabled in the scripts?
A new environment macro
@__FEATURES__
will be add toPkg
and it can be used to decide whether to precompile/use part of the code, e.gThis macro contains code that looks up current environment and returns a vector of string. e.g
If this package is developed in env
v1.0
, then it will look upv1.0/Manifest.toml
which contains the enabled features and put that to@__FEATURES__
.Cases
If this is implemented, we can install Flux in the following syntax to choose different platform.
similar for many other packages, it will become more explicit and elegant.
Potential Problems
The extra key word
--features
inadd
will need to be the last argument, or the CLI cannot recognize multiple features.