conda / rattler

Rust crates to work with the Conda ecosystem.
BSD 3-Clause "New" or "Revised" License
252 stars 51 forks source link

feat: Add support for `CONDA_OVERRIDE_CUDA` #818

Closed SobhanMP closed 4 weeks ago

SobhanMP commented 1 month ago

Description

Hi, I know this is terrible, not elegant, etc, but I do think that having support for something in the spirit of CONDA_OVERRIDE_CUDA is necessary on academic HPC environments. Reasons include

I'm not really used to writing rust, so would be greatful for some guidance.

Thanks

wolfv commented 1 month ago

I think this would also be good for all the other virtual packages as a way to hot-fix certain scenarios that we haven't covered yet (#815 for example).

baszalmstra commented 1 month ago

My idea would be to add functions to the virtual package implementations. Lets take CUDA as an example, we would add these functions to the Cuda impl:

impl Cuda {
    pub fn from_env_var_name(env_var_name: &str) -> Option<Self> {
        let version_str = env::var(env_var_name)?;
        // Extract versions
        ...
    }

    pub fn from_default_env_var() -> Option<Self> {
        Self::from_env_var_name("CONDA_OVERRIDE_CUDA")
    }

    ...
}

You can then write:

let cuda = Cuda::from_default_env_var().or_else(Cuda::current);

To get the CUDA version of the system based on env var, or if not present by detecting it.

Similarly a function VirtualPackage::from_default_env_vars could be added.

I think this allows:

Which I think hits the sweet spot.

And indeed as @wolfv said, it would be nice to add these for the other virtual packages as well.

SobhanMP commented 1 month ago

oh, yeah that looks great!

SobhanMP commented 1 month ago

Does this make sense as a rough draft?

SobhanMP commented 1 month ago

I think there is one issue, finding the environment variable and it being empty is different than the environment variable not being present, should the type be Option<Option<T>>? first one option showing that the env was found the second one that the env was none?

SobhanMP commented 1 month ago

I think everything should have been handled gracefully.

SobhanMP commented 1 month ago

Do we want to override the glibc runtime? like CONDA_OVERRIDE_GLIBC="2@musl", same for Linux and Archspec. We could also do unix and win but all of these are not overridable by conda.

iamthebot commented 4 weeks ago

Do we want to override the glibc runtime? like CONDA_OVERRIDE_GLIBC="2@musl", same for Linux and Archspec. We could also do unix and win but all of these are not overridable by conda.

Suggestion-- could this be done in a followup PR? Landing CONDA_OVERRIDE_CUDA support is already immensely valuable and likely there will be a long tail of less common overrides needed.

iamthebot commented 4 weeks ago

Anything else needed to get this over the finish line? This is a blocker for us switching fully over to rattler so happy to dedicate to some time to any remaining blockers in getting this merged.

SobhanMP commented 4 weeks ago

This should solve some of the failure, pre-commit is not ran by default

iamthebot commented 4 weeks ago

Are there any implications for pyrattler from this PR btw? Anything new that should be exposed?

SobhanMP commented 4 weeks ago

I don't think pyrattler has any implication for pixi, so I'd rather this be merged w/o py-rattler support. My understanding is that in py-rattler/src/virtual_package.rs the PyVirtualPackage needs to expose some more code, the problem is that this either requires either 1- A special classs for virtual packages that can be overriden (there is already one for arch), 2- that all virtual pacakges have override, or 3- using Cuda::from_default_env_var.unwrap_or(Cuda::current().into()).unwrap() in the VirtualPackage::current or try_detect_virtual_packages instead of Cuda::current()

But I think that someone else has probably a better idea.

SobhanMP commented 4 weeks ago

@baszalmstra It's very inconvenient that the checks require maintainer auth.

baszalmstra commented 4 weeks ago

Yes, but once your first PR lands that restriction is lifted 👍

SobhanMP commented 4 weeks ago

oh nice!

baszalmstra commented 4 weeks ago

I think it would be good to also expose this functionality to the python bindings, but we can do that in another PR.

Thanks for your first contribution @SobhanMP !

SobhanMP commented 4 weeks ago

@baszalmstra I think this first requires figuring out https://github.com/conda/rattler/pull/818#issuecomment-2297850077 . I think option 2 makes the most sense.

SobhanMP commented 4 weeks ago

The next thing is where this should be used. Should try_detect_virtual_packages be patched? Should it take some structure to rename overrides?