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

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.00%. Comparing base (9afb325) to head (974a77f).
⚠️ Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1993      +/-   ##
==========================================
+ Coverage   59.98%   60.00%   +0.02%     
==========================================
  Files         163      163              
  Lines       20118    20121       +3     
  Branches     3519     3519              
==========================================
+ Hits        12067    12074       +7     
+ Misses       7230     7226       -4     
  Partials      821      821              

☔ View full report in Codecov by Sentry.
📢 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?