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.

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

feat: Add support for CONDA_OVERRIDE_CUDA #818

Merged
merged 11 commits into from
Aug 20, 2024
Merged

Conversation

SobhanMP
Copy link
Contributor

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

  • only having internet access on the login node with no gpu
  • launching jobs with submitit on login nodes with no gpu.

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

Thanks

@SobhanMP SobhanMP changed the title Add support for CONDA_OVERRIDE_CUDA [feat] Add support for CONDA_OVERRIDE_CUDA Aug 14, 2024
@SobhanMP SobhanMP changed the title [feat] Add support for CONDA_OVERRIDE_CUDA feat: Add support for CONDA_OVERRIDE_CUDA Aug 14, 2024
@@ -26,10 +27,14 @@ pub fn cuda_version() -> Option<Version> {
.clone()
}

const CUDA_OVERRIDE_ENV: &str = "CONDA_OVERRIDE_CUDA";
/// Attempts to detect the version of CUDA present in the current operating system by employing the
/// best technique available for the current environment.
pub fn detect_cuda_version() -> Option<Version> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function should be pure functional without environmental side-effects.

Instead I propose we add some sort of from_env function to the appropriate virtual packages that allow users to choose whether they want this behavior. Or a or_override_with_env_var or something along those lines.

We should also debate the environment variable name. I believe conda has a different name? Im also not sure whether we should hardcode that in rattler itself.

Copy link
Contributor Author

@SobhanMP SobhanMP Aug 14, 2024

Choose a reason for hiding this comment

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

This function should be pure functional without environmental side-effects.

But the function is already not pure because it runs shell commands/dlopens libraries.

Instead I propose we add some sort of from_env function to the appropriate virtual packages that allow users to choose whether they want this behavior. Or a or_override_with_env_var or something along those lines.

I'm not sure I understand. where would this be? in the config?

We should also debate the environment variable name.

This is the name see here

@wolfv
Copy link
Contributor

wolfv commented Aug 15, 2024

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
Copy link
Collaborator

baszalmstra commented Aug 15, 2024

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:

  • Users of the library to decide whether they want to allow environment overrides.
  • Users can specify custom environment variables if they want to.
  • Users can use the default environment variable names.

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
Copy link
Contributor Author

oh, yeah that looks great!

@SobhanMP
Copy link
Contributor Author

Does this make sense as a rough draft?

Copy link
Collaborator

@baszalmstra baszalmstra left a comment

Choose a reason for hiding this comment

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

Yeah that looks a lot better! I left a few comments.

crates/rattler_virtual_packages/src/lib.rs Outdated Show resolved Hide resolved
crates/rattler_virtual_packages/src/lib.rs Outdated Show resolved Hide resolved
@SobhanMP
Copy link
Contributor Author

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
Copy link
Contributor Author

I think everything should have been handled gracefully.

@SobhanMP
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Collaborator

@baszalmstra baszalmstra left a comment

Choose a reason for hiding this comment

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

This looks good! Ill take a closer look at the CI failure tomorrow!

@SobhanMP
Copy link
Contributor Author

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

@iamthebot
Copy link
Contributor

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

@SobhanMP
Copy link
Contributor Author

SobhanMP commented Aug 20, 2024

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
Copy link
Contributor Author

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

@baszalmstra
Copy link
Collaborator

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

@SobhanMP
Copy link
Contributor Author

oh nice!

@baszalmstra baszalmstra merged commit 721a6c1 into conda:main Aug 20, 2024
15 checks passed
@baszalmstra
Copy link
Collaborator

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 !

@baszalmstra baszalmstra mentioned this pull request Aug 19, 2024
@SobhanMP
Copy link
Contributor Author

@baszalmstra I think this first requires figuring out #818 (comment) . I think option 2 makes the most sense.

@SobhanMP
Copy link
Contributor Author

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

baszalmstra added a commit that referenced this pull request Sep 2, 2024
## 🤖 New release
* `rattler_conda_types`: 0.27.2 -> 0.27.3
* `rattler_package_streaming`: 0.22.3 -> 0.22.4
* `rattler_lock`: 0.22.20 -> 0.22.21
* `rattler_solve`: 1.0.3 -> 1.0.4
* `rattler_virtual_packages`: 1.0.4 -> 1.1.0
* `rattler_index`: 0.19.24 -> 0.19.25
* `rattler`: 0.27.6 -> 0.27.7
* `rattler_cache`: 0.1.8 -> 0.1.9
* `rattler_shell`: 0.21.6 -> 0.21.7
* `rattler_repodata_gateway`: 0.21.8 -> 0.21.9

<details><summary><i><b>Changelog</b></i></summary><p>

## `rattler_conda_types`
<blockquote>

##
[0.27.3](rattler_conda_types-v0.27.2...rattler_conda_types-v0.27.3)
- 2024-09-02

### Added
- add edge case tests for `StringMatcher`
([#839](#839))
</blockquote>

## `rattler_package_streaming`
<blockquote>

##
[0.22.4](rattler_package_streaming-v0.22.3...rattler_package_streaming-v0.22.4)
- 2024-09-02

### Added
- Add support for `CONDA_OVERRIDE_CUDA`
([#818](#818))

### Fixed
- zip large files compression
([#838](#838))
</blockquote>

## `rattler_lock`
<blockquote>

##
[0.22.21](rattler_lock-v0.22.20...rattler_lock-v0.22.21)
- 2024-09-02

### Added
- Add support for `CONDA_OVERRIDE_CUDA`
([#818](#818))
</blockquote>

## `rattler_solve`
<blockquote>

##
[1.0.4](rattler_solve-v1.0.3...rattler_solve-v1.0.4)
- 2024-09-02

### Fixed
- Redact spec channel before comparing it with repodata channel
([#831](#831))

### Other
- Remove note that only libsolv is supported
([#832](#832))
</blockquote>

## `rattler_virtual_packages`
<blockquote>

##
[1.1.0](rattler_virtual_packages-v1.0.4...rattler_virtual_packages-v1.1.0)
- 2024-09-02

### Added
- start adding interface to override
([#834](#834))
- Add support for `CONDA_OVERRIDE_CUDA`
([#818](#818))

### Other
- make virtual package overrides none by default consistently
([#842](#842))
</blockquote>

## `rattler_index`
<blockquote>

##
[0.19.25](rattler_index-v0.19.24...rattler_index-v0.19.25)
- 2024-09-02

### Other
- release ([#824](#824))
</blockquote>

## `rattler`
<blockquote>

##
[0.27.7](rattler-v0.27.6...rattler-v0.27.7)
- 2024-09-02

### Other
- updated the following local packages: rattler_conda_types,
rattler_package_streaming
</blockquote>

## `rattler_cache`
<blockquote>

##
[0.1.9](rattler_cache-v0.1.8...rattler_cache-v0.1.9)
- 2024-09-02

### Other
- updated the following local packages: rattler_conda_types,
rattler_package_streaming
</blockquote>

## `rattler_shell`
<blockquote>

##
[0.21.7](rattler_shell-v0.21.6...rattler_shell-v0.21.7)
- 2024-09-02

### Other
- updated the following local packages: rattler_conda_types
</blockquote>

## `rattler_repodata_gateway`
<blockquote>

##
[0.21.9](rattler_repodata_gateway-v0.21.8...rattler_repodata_gateway-v0.21.9)
- 2024-09-02

### Other
- updated the following local packages: rattler_conda_types
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/MarcoIeni/release-plz/).
kelvinou01 pushed a commit to kelvinou01/rattler that referenced this pull request Sep 3, 2024
## 🤖 New release
* `rattler_conda_types`: 0.27.2 -> 0.27.3
* `rattler_package_streaming`: 0.22.3 -> 0.22.4
* `rattler_lock`: 0.22.20 -> 0.22.21
* `rattler_solve`: 1.0.3 -> 1.0.4
* `rattler_virtual_packages`: 1.0.4 -> 1.1.0
* `rattler_index`: 0.19.24 -> 0.19.25
* `rattler`: 0.27.6 -> 0.27.7
* `rattler_cache`: 0.1.8 -> 0.1.9
* `rattler_shell`: 0.21.6 -> 0.21.7
* `rattler_repodata_gateway`: 0.21.8 -> 0.21.9

<details><summary><i><b>Changelog</b></i></summary><p>

## `rattler_conda_types`
<blockquote>

##
[0.27.3](conda/rattler@rattler_conda_types-v0.27.2...rattler_conda_types-v0.27.3)
- 2024-09-02

### Added
- add edge case tests for `StringMatcher`
([conda#839](conda#839))
</blockquote>

## `rattler_package_streaming`
<blockquote>

##
[0.22.4](conda/rattler@rattler_package_streaming-v0.22.3...rattler_package_streaming-v0.22.4)
- 2024-09-02

### Added
- Add support for `CONDA_OVERRIDE_CUDA`
([conda#818](conda#818))

### Fixed
- zip large files compression
([conda#838](conda#838))
</blockquote>

## `rattler_lock`
<blockquote>

##
[0.22.21](conda/rattler@rattler_lock-v0.22.20...rattler_lock-v0.22.21)
- 2024-09-02

### Added
- Add support for `CONDA_OVERRIDE_CUDA`
([conda#818](conda#818))
</blockquote>

## `rattler_solve`
<blockquote>

##
[1.0.4](conda/rattler@rattler_solve-v1.0.3...rattler_solve-v1.0.4)
- 2024-09-02

### Fixed
- Redact spec channel before comparing it with repodata channel
([conda#831](conda#831))

### Other
- Remove note that only libsolv is supported
([conda#832](conda#832))
</blockquote>

## `rattler_virtual_packages`
<blockquote>

##
[1.1.0](conda/rattler@rattler_virtual_packages-v1.0.4...rattler_virtual_packages-v1.1.0)
- 2024-09-02

### Added
- start adding interface to override
([conda#834](conda#834))
- Add support for `CONDA_OVERRIDE_CUDA`
([conda#818](conda#818))

### Other
- make virtual package overrides none by default consistently
([conda#842](conda#842))
</blockquote>

## `rattler_index`
<blockquote>

##
[0.19.25](conda/rattler@rattler_index-v0.19.24...rattler_index-v0.19.25)
- 2024-09-02

### Other
- release ([conda#824](conda#824))
</blockquote>

## `rattler`
<blockquote>

##
[0.27.7](conda/rattler@rattler-v0.27.6...rattler-v0.27.7)
- 2024-09-02

### Other
- updated the following local packages: rattler_conda_types,
rattler_package_streaming
</blockquote>

## `rattler_cache`
<blockquote>

##
[0.1.9](conda/rattler@rattler_cache-v0.1.8...rattler_cache-v0.1.9)
- 2024-09-02

### Other
- updated the following local packages: rattler_conda_types,
rattler_package_streaming
</blockquote>

## `rattler_shell`
<blockquote>

##
[0.21.7](conda/rattler@rattler_shell-v0.21.6...rattler_shell-v0.21.7)
- 2024-09-02

### Other
- updated the following local packages: rattler_conda_types
</blockquote>

## `rattler_repodata_gateway`
<blockquote>

##
[0.21.9](conda/rattler@rattler_repodata_gateway-v0.21.8...rattler_repodata_gateway-v0.21.9)
- 2024-09-02

### Other
- updated the following local packages: rattler_conda_types
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/MarcoIeni/release-plz/).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants