Draft: feat: package orderers as configurable plugins with config-arg-passing - #2165
Conversation
Add tests for: - from_pod module-level function round-trip (the code path the plugin refactor will change) - isinstance checks against imported class names (will catch function-shim breakage) - get_orderer default fallback to SortedOrder(descending=True) Signed-off-by: Stephen Mackenzie <maxnbk@users.noreply.github.com>
…ySoftwareFoundation#1787) Move the five built-in package orderers (NullPackageOrder, SortedOrder, PerFamilyOrder, VersionSplitPackageOrder, TimestampPackageOrder) from src/rez/package_order.py into src/rezplugins/package_order/ as proper rez plugins. Register PackageOrderPluginType in plugin_managers.py. Replace PR AcademySoftwareFoundation#1787's function shims with lazy module-level __getattr__ aliases (PEP 562) so isinstance, from_pod, and type(x) is checks all work, while avoiding circular imports at module load time. Keep _orderers dict and register_orderer() as legacy fallback, marked with deprecation comments. Original work by Robert Minsk (PR AcademySoftwareFoundation#1787). Rebased, conflicts resolved, and fixed by Stephen Mackenzie. Signed-off-by: Robert Minsk <robertminsk@yahoo.com> Signed-off-by: Stephen Mackenzie <maxnbk@users.noreply.github.com>
Add tests for: - Plugin system loads all five built-in orderers - to_pod/from_pod round-trip through the plugin system - Legacy register_orderer fallback - REZ_PACKAGE_ORDERERS_JSON end-to-end configuration (simulated via config.override since the test framework uses a locked config) - Cache invalidation for config changes - Test-only kwarg-orderer validates the AcademySoftwareFoundation#1706/AcademySoftwareFoundation#1709 pattern Signed-off-by: Stephen Mackenzie <maxnbk@users.noreply.github.com>
Add documentation for: - REZ_PACKAGE_ORDERERS_JSON environment variable configuration - Plugin-based custom orderer approach (preferred over register_orderer) - Cache invalidation for runtime config changes Signed-off-by: Stephen Mackenzie <maxnbk@users.noreply.github.com>
6022842 to
1951153
Compare
Update plugin files to match main's post-AcademySoftwareFoundation#1761 code: - Add type annotations (Version, SupportsLessThan, Package, etc.) - Add missing imports (each plugin now self-contained) - Use 'is' instead of '==' for type comparisons (no E721) - Fix E203 whitespace before ':' in soft_timestamp.py slice - Match __eq__, to_pod, from_pod, __init__ signatures from main Signed-off-by: Stephen Mackenzie <maxnbk@users.noreply.github.com>
…r resets test_plugin_manager resets the plugin manager between test runs, which deletes rezplugins.* from sys.modules and re-registers all plugin types. This creates new class objects for orderer plugins. Tests that imported orderer classes at module level (from rez.package_order import SortedOrder) were bound to stale class objects, causing isinstance and __eq__ failures when the full test suite ran in CI. Fix: resolve orderer classes in setUp() via _find_orderer() so each test method gets the current class objects. Signed-off-by: Stephen Mackenzie <maxnbk@users.noreply.github.com>
1951153 to
39a2d37
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2165 +/- ##
==========================================
+ Coverage 61.29% 61.49% +0.19%
==========================================
Files 164 170 +6
Lines 20568 20639 +71
Branches 3575 3581 +6
==========================================
+ Hits 12607 12691 +84
+ Misses 7089 7075 -14
- Partials 872 873 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…methods Covers: sort_key with VersionRange/None/invalid types, FallbackComparable fallback path, _find_orderer with unknown name, __getattr__ AttributeError, register_orderer valid/invalid, PackageOrderList dirty-flag methods, to_pod/from_pod round-trip, PerFamilyOrder sort_key_implementation with default_order fallback and RuntimeError path. Raises orderer test coverage from 85% to 97%. Signed-off-by: Stephen Mackenzie <maxnbk@users.noreply.github.com>
JeanChristopheMorinPerso
left a comment
JeanChristopheMorinPerso
left a comment
There was a problem hiding this comment.
I think I'm generally good with this but would like to see some changes before we merge this. I think it's going in the right direction. Note that this review was a manual review from me with no LLM assistance.
| packages=data.get("packages"), | ||
| ) | ||
| if name in _LEGACY_ORDERER_NAMES: | ||
| return _find_orderer(_LEGACY_ORDERER_NAMES[name]) |
There was a problem hiding this comment.
I think we should raise a deprecation warning and pave the road for removing this fallback in a few releases. We can use rez.deprecations.warn.
| """ | ||
| test completions | ||
| """ | ||
|
|
There was a problem hiding this comment.
I would get rid of all the formatting changes if possible. We can make the formatting changes separately in another PR.
|
|
||
| try: | ||
| return plugin_manager.get_plugin_class("package_order", name) | ||
| except RezPluginError: |
There was a problem hiding this comment.
Might be worth introducing a new exception class that subclasses RezPluginError to be more specific about what the error is, like RezPluginNotFound or something like that? Doing this wouldn't break user and it would improve our plugin system just a little bit.
| Use this when runtime configuration (e.g. REZ_PACKAGE_ORDERERS_JSON) | ||
| has changed and you want the new config to take effect. Note that | ||
| the config-level cache must also be cleared via | ||
| ``config._uncache("package_orderers")``. |
There was a problem hiding this comment.
config._uncache("package_orderers")
This makes me feel uneasy as it's a private method. Also, why is this method needed? Are we doing something similar in other places and this just keeps things consistent? API users have access to rez.config.config.copy/override/remove_override to change the condig at runtime...
| if orderer is None: | ||
| # default ordering is version descending | ||
| orderer = SortedOrder(descending=True) | ||
| sorted_order = _find_orderer("sorted") |
There was a problem hiding this comment.
| sorted_order = _find_orderer("sorted") | |
| sorted_order = _find_orderer("SortedOrder") |
to avoid relying on the module getattr.
| # Orderers registered at runtime via register_orderer(). This is the API-based | ||
| # registration pathway, used as fallback by _find_orderer when an orderer is | ||
| # not found in the plugin system. | ||
| _orderers = {} |
There was a problem hiding this comment.
This is already defined at line 196.
|
|
||
| returns: | ||
| bool: True if successfully registered, else False. | ||
| """ |
There was a problem hiding this comment.
We should add a deprecation warning to nudge users towards the plugins.
| type_name = "command" | ||
|
|
||
|
|
||
| class PackageOrderPluginType(RezPluginType): |
There was a problem hiding this comment.
We need to update the plugin docs with this new plugin type: https://github.com/AcademySoftwareFoundation/rez/blob/main/docs/source/plugins.rst?plain=1#L14
|
|
||
| This is the API-based registration pathway, useful for dynamic or | ||
| programmatic orderer registration. For persistent, facility-wide | ||
| orderers, prefer the plugin system (rezplugins/package_order/). |
There was a problem hiding this comment.
| orderers, prefer the plugin system (rezplugins/package_order/). | |
| orderers, it is preferred to use :doc:`plugins <plugins>`. |
|
|
||
| Package orderers can be configured in the ``rezconfig.py`` via the :data:`package_orderers` setting. | ||
|
|
||
| Environment Variable Configuration |
There was a problem hiding this comment.
I think it might be better if we document all the JSON environment variables. It feels weird that we would document this one specifically but not the others. Also, does the note about changing the config at runtime applicable to all settings? If so, it would be preferable to have this documented in the config docs.
There was a problem hiding this comment.
Here's a PR that adds documentation for JSON env vars.
|
Oh, I'm also wondering if we should deprecate |
3 participants
** WIP, DO NOT REVIEW YET :) **
Summary
Rebases and fixes #1787 by @cfxegbert
Moves the 5 built-in package-orderers from
src/rez/package_order.pytosrc/rezplugins/package_order/as proper rez plugins.Adds runtime configuration support via
REZ_PACKAGE_ORDERERS_JSONand cache invalidation, creating a platform for PRs #1706 + #1709 to land in a usable manner with minimal changes.What changed (on top of the PR #1787 rebase+fixing)
PackageOrderPluginTypeinplugin_managers.py__getattr__lazy aliases, soisinstance,from_pod,type(x) ischecks all work against the imported class names, while avoiding a circular import that module-level aliases would cause (plugin files import from package_order.py, which would call_find_orderer-> plugin system at load time)register_orderer()and the_orderersdict as a means to subclass/apply orderers programmatically.Runtime configuration of Package Orderers
REZ_PACKAGE_ORDERERS_JSONworks end-to-end using the existing_JSONenv-var configuration pathway that then feeds intoconfig.package_orderers->PackageOrderList.singleton->from_pod()PackageOrderList.clear_singleton_cache()classmethod to invalidate the cached singleton, so runtime config changes can take effect without restarting, or API-based-usage of the feature can be correctly managed.Documentation
REZ_PACKAGE_ORDERERS_JSONusage section todocs/source/package_orderers.rstregister_orderer()Design Notes
SortedOrder = _find_orderer('sorted')at module level, but this causes a circular import (package_order.py->_find_orderer()-> plugin system ->rezplugins/package_order/sorted.py->from rez.package_order import PackageOrder-> still importing). The__getattr__approach defers resolution to the first attribute access, breaking the cycle.REZ_PACKAGE_ORDERERS_JSONpath by parsing JSON and usingconfig.override()which is what the_JSONpath does internally.Testing
from_podround-trip,isinstanceagainst imported classes,get_ordererdefault fallbackto_pod/from_podround-trip through the plugin system, legacyregister_ordererfallback, runtime config viaconfig.override(simulating the_JSONpath), cache invalidation, and a test-only kwarg-orderer that validates the pattern used in Add a package orderer that matches Python's version specifier spec (PEP440) #1706 / Added CustomPackageOrder #1709 (constructor kwargs configured at runtime).Credit
Original refactor by @cfxegbert . Rebased on to current main, conflicts resolved, backward-compat shims fixed, and runtime configuration support added by me.
Follow-on work (not here)
PackageOrderclass names (and other namings/imports/etc) toPackageOrdererfor making-more-sense?rezpluginsto join the rest.Disclosure: This PR was assisted by GLM-5.2 for docstrings, documentation, and test generation.