Improvement move pkg cache fs calls to repo plugin

[vanridal]

This PR is an attempt to decouple the package cache operation to the repository plugins

This can allow users to customize how the package cache touches the filesystem on a per repository bases either via the cache_variant() call, or on the variant resource

Does this help with the artifact repo direction, could some remote repos use the current cache system via moving these functions to the repository plugin?

I personally would like a way to customize pkg cache operation via a custom filesystem plugin to optimize cache speeds

[codecov]

Codecov Report

❌ Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 61.32%. Comparing base (5c598c5) to head (feb7a50).

Files with missing lines	Patch %	Lines
src/rez/package_repository.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1993      +/-   ##
==========================================
+ Coverage   61.29%   61.32%   +0.02%     
==========================================
  Files         164      164              
  Lines       20568    20582      +14     
  Branches     3575     3577       +2     
==========================================
+ Hits        12607    12621      +14     
- Misses       7089     7090       +1     
+ Partials      872      871       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[vanridal]

@JeanChristopheMorinPerso
Thought I would share, this commit was the most minimal change we had to make for us to implement artifact cache at our facility.
Moving the call to PackageRepository class allowed for the Repository plugin to dictate how rez should cache the repos payloads. Whether handle it itself (our current method) or could defer to a artifact repo it has relationship with or even call upon another plugin, say a copy plugin that can interface to its payload format.

I would like to see this adopted in the rez, as I think for such a small change it does empower anyone to take ownership of the cache copy method without the need of too much refactor of rez's core code.

[crowecawcaw]

This simple change would unlock Rez adoption for a use case I'm looking at as well. Is there an opportunity to move this forward? I'm happy to pick it up if that would help - rebasing and adding unit tests. It looks like a great enabler for remote payload backends like S3 or other object stores. Super minimal, simple to use, follows existing patterns of providing overridable methods for plugin extensions.

[maxnbk]

It would be a good idea.
I've done some initial review today (sorry, I'm definitely trying to get some reviews on some PRs that have been waiting for them for a long time, but the backlog is deep).

My review findings are:

1. Overall, it's a good forward step towards better package-caching utility and pluginification. There are some minor issues I would like to see addressed however, either as a matter of safety/consistency, or as a matter of being explicit about the architecture moving forward.
2. cache_variant is not abstract. There would be a silent no-op for any non-filesystem repo. The base class profiles a default implementation (the copytree), which works for filesystem repos where variant.root is a local path, but since we want to push forward remote/artifact repos (which this PR is helping to move the goalpost on), variant.root may not be a local filesystem path, or may not exist at all. A remote repo that doesn't override cache_variant would silently get copytree called on a non-existent path, producing a confusing FileNotFoundError instead of a clear "not supported" message. I would therefore recommend that we make cache_variant raise NotImplementedError by default (similar to how get_package_payload_path does), forcing each repo plugin to explicitly opt-in. If not this, then document that it assumes a local filesystem path, and have the filesystem repo override it explicitly (moving the implementation, not duplicating it).
3. The original variant_root was gotten with getattr which returns None if the attribute doesn't exist. variant.root is an @cached_property on VariantResource that calls self._root(). If _root() returns None, variant.root would raise AttributeError or return None. Basically, the failure mode is different than getattr with a default. In practice this probably doesn't matter because add_variant already validates variant_root before reaching the copy, but the cache_variant method on the base class doesn't have that protection. It's a public method that could be called independently. My suggestion is that cache_variant is at the wrong abstraction level. The method is on PackageRepository (the repo), but it receives both the variant and the location (cache destination). The repo knows how to read from itself, but the cache knows where to write. The current signature conflates the source and destination concerns. Cleaner split might be PackageRepository.get_variant_payload(variant) -> Iterator[bytes] or -> path, where the repo provides the payload, PackageCache handles writing to the cache location. Alternatively, if the goal is for the repo to control the full copy, cache_variant(self, variant, cache_rootpath) which is the current approach, but document that the repo owns the full source->dest transfer. It's not that this approach is bad, it's just pragmatic, but worth documenting clearly.
4. Not a bug in this PR, but a constraint that cache_variant implementations need to be aware of: The original copytree uses defaults, so, symlinks=False, _copy_function=copy2. This is fine for filesystem repos, but the caches get_variant_size explicitly follows symlinks to compute size. If a custom repo overrides cache_variant and uses symlinks=True, the cached size would mismatch the actual cached payload. Additionally, the .copying-* sentinel mechanism and the _while_copying thread assume the copy takes a non-trivial amount of time. A remote repo that does a streaming download might finish instantly or take much longer, but the timeout/stall detection is calibrated for filesystem copy speeds. (Technically this is a problem today, but I am trying to be proactive with how we make these adjustments...)

Some concrete suggestions/nitpicks:

1. A docstring on the new method would be nice.
2. If cache_variant was overridden by all plugins then import shutil is dead import code for those plugins. Not a real problem but the import could be moved to the method level or the default should delegate to a utility function.
3. The tests still pass because effectively the same code is being exercised. However, tests could verify a custom repo plugin can override cache_variant, verify that the base class default works without an override, and verify error handling when cache_variant fails.

I would only really consider the "default implementation safety" items as important to address, the docstring as an easy add, and the rest are good followups.

[vanridal]

Hi @maxnbk
Thx for looking at this, I will address the points you have listed.
Question on point 3.
Looking at it with new eyes, The goal here is to give ownership of the caching action over to the repo plugin, so the repo can dictate how to read/write their payloads.
Looking at the api of the Variant class, would making the call via variant.cache(path) be another avenue here, mirroring the variant.install() mechanism. Where the variant.cache() call would over see passing the Variant resource to its repository cache_variant call?

[vanridal]

@maxnbk
I've given this PR a little refactor, It started as a simple attempt for consideration and to get feedback, so now ive given it more attention to the structure of the api.
Just the note of the goals im providing with this PR.

1. Allow the copy action to be implemented on a repo plugin, allow consumers to tailor the copy action to a more performant method then what the standard python api can provide or even choose to design in other ways, via the plugin.
2. Facilitate moving forward, rez supporting remote repositories, the action of reading a payload is only known to the repo, S3 say, payload data is streamed to a filesystem via variant.copy_payload interface.

Points on design

• PkgCache now calls method variant.copy_payload(path: str), for the action of interacting with a filesystem.
• copy_payload maps its call to the interface on a repository, copy_variant_payload(resource, path)
• Ive attempted to mirror variant.install() --> repo.install_variant()
• Default copy_variant_payload on the base class will raise NotImplementedError.
• Ive chosen to avoid using the name 'cache' for 'copy' in the function names to keep it more generic to the act of coping data to a standard file system.
• This interface could be expanded to rez-cp, rez-mv.
  Copy variant from remote repo to filesystem repo

What's not considered in regards to remote repos in this PR

• PkgCache.add_variant also touches the filesystem in other places, these calls would need another standard interface to allow caching of packages from remote repos.

  • Checks is variant.root is existing path, --> perhaps a variant.exists() call
  • Does a stat on both variant payload path and cache dir to decern if same filesystem --> perhaps a repo could have a repo.is_remote() and skip doing a stat.

• how to copy payload data 'to' a remote repo

Please let me know your thoughts, and if you think this is the right direction for rez to take.