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

refactor: put PackageCache into an enum in preparation for a multi-layered package cache #843

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

refactor: put PackageCache into an enum in preparation for a multi-layered package cache #843

wants to merge 8 commits into from

Conversation

kelvinou01
Copy link
Contributor

@kelvinou01 kelvinou01 commented Sep 3, 2024 •
edited
Loading

Description

As per #43, we want to implement layered package caches for Rattler. Putting PacakgeCache into an enum allows switching between implementations (SingletonPackageCache and LayeredPackageCache)

I'm considering both traits and enums. Leaning towards enum because:

  • My impression is that we don't allow downstream users to add their own PackageCache types, thus a closed set of types (enum) is more suitable.
  • Enums are more performant

Stuff to note

  • This will be a breaking change
  • To let PackageCache be public, I had to make PackageCacheInner public as well (even though inner is a private struct field), not sure if there's a way not to.

kelvinou01 and others added 3 commits September 3, 2024 15:03
@kelvinou01
Refactor PackageCache into an enum to prepare for layered package cac…
7b1c7e2 
…he feature
@baszalmstra @kelvinou01
chore: release (conda#836)
b428ca2 
## 🤖 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/).
@kelvinou01
Change constructor calls to new_singleton
2b04cf5
@baszalmstra
Copy link
Collaborator

Hey! Thanks for making this PR! Im wondering what your design considerations are. Why have two separate implementations? Isnt a non-layered cache just cache with one layer?

@kelvinou01
Copy link
Contributor Author

kelvinou01 commented Sep 3, 2024 •
edited
Loading

Isnt a non-layered cache just cache with one layer?

Hey @baszalmstra! I was actually referencing this

We could introduce a trait for PackageCache that is implemented for both the current implementation as well as a layered version.

Maybe we can allow users to pick between the singleton and layered implementations, while we polish the latter? Or should we just create a layered implementation and have it be shipped to users (with the default being a cache with one layer)

@baszalmstra
Copy link
Collaborator

baszalmstra commented Sep 3, 2024 •
edited
Loading

Maybe we can allow users to pick between the singleton and layered implementations, while we polish the latter? Or should we just create a layered implementation and have it be shipped to users (with the default being a cache with one layer)

Yeah, I think (in hindsight) that makes the most sense.

FYI I'm also about to merge a relatively large refactor of the PackageCache in #837.

@kelvinou01
Copy link
Contributor Author

Thanks for the heads up on the PR!

Also just to be sure, which idea did you think made the most sense, using the enum or not using it?

@baszalmstra
Copy link
Collaborator

Also just to be sure, which idea did you think made the most sense, using the enum or not using it?

I think a single implementation makes more sense. But if you feel uncertain about the implementation using an enum is fine for me as well.

@baszalmstra
Copy link
Collaborator

#837 was just merged, you might have a few conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kelvinou01 @baszalmstra