Abstract away plugin lookup and querying #5
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Refactor `get_variant_hashes_by_priority()` to reuse the list of plugin names that is already included in the result of `_query_variant_plugins()` rather than iterating over entry points again.
Add a `PluginLoader` class that handles loading and querying plugins. This removes some duplicate code, prepares for plugins having more than one method in the future, and enables using a single cached plugin list, rather than repeatedly querying and instantiating them.
|
I'm all for this change - however you have 2 caching mechanisms and I don't think that's very useful.
I think that function Please preserve |
|
Yes, right now the plugin API provides only one method, so there's only one use case for |
4 tasks
|
(I've switched to a static method) |
|
Resolved merge conflicts after |
DEKHTIARJonathan
approved these changes
Mar 24, 2025
DEKHTIARJonathan
merged commit 91ee0e5
into
wheelnext:variant-json
4 of 6 checks passed
DEKHTIARJonathan
added a commit
that referenced
this pull request
Mar 25, 2025
* Implement getting sorted variants aided by `variants.json` Add a `variants_json` argument to `get_variant_hashes_by_priority()` that enables the function to use variant definitions from `variants.json` instead of evaluating all possible combinations. This can yield significant speedup and lower resource use if only a handful of variants need to be considered. The function is implemented without any plugin API changes. After all, the current API should be simpler for plugin authors, and generating separate variant lists should not pose a performance problem. Implemention-wise, the function first converts all variant descriptions from `variants.json` into `VariantDescription` classes. Then, the list of variants is fitlered to include only variants that match configurations obtained from plugins. Finally, they are sorted using the configuration order. I am not 100% sure about the sorting algorithm, but it seems to yield exactly the same order at least for the `dummy-project` wheels. The first element of the sort key is the number of metas in the variant, to prioritize variants that matched more items. It is followed by priorities (indices in configuration order) of all metas, ordered from the most preferred to the least preferred. * Fix crash when a plugin is not installed * Warn if providers or variant keys are missing * Add a (failing) round-trip test for variants.json-based sorting * Add CI workflow * Fix variant sorting algorithm * Add fuzzing tests (failing now) * Include the known-failing example explicitly * Fix sorting algorithm again * Dependency Update * Relax whitespace requirements in `VariantMeta.from_str()` (#4) Allow any (or no) whitespace between `::` separators in `VariantMeta.from_str()`, in order to make it more feasible for parsing user input. Unless I'm missing something, there should be no need to strictly ensure that input is strictly formatted as `A :: B :: C` (rather than, say, `A::B::C`), as long as we properly format output. * [Removal of `attrs`] Replacing with `dataclass` (#3) * [Transition to dataclass] `meta.py` file * [Transition to dataclass] `config.py` file * attrs removed * Cleanup * Abstract away plugin lookup and querying (#5) * Remove duplicate plugin querying in get_variant_hashes_by_priority() Refactor `get_variant_hashes_by_priority()` to reuse the list of plugin names that is already included in the result of `_query_variant_plugins()` rather than iterating over entry points again. * Abstract away plugin lookup and querying Add a `PluginLoader` class that handles loading and querying plugins. This removes some duplicate code, prepares for plugins having more than one method in the future, and enables using a single cached plugin list, rather than repeatedly querying and instantiating them. * Replace `load_plugins()` with `create()` static method * [Switch to Singleton] --------- Co-authored-by: Jonathan DEKHTIAR <[email protected]> * plugins: Rename `run()` to `get_supported_configs()`, add tests (#8) Rename the `run()` API to `get_supported_configs()`, to prepare for adding other plugin methods. Fix bugs introduced while introducing the singleton pattern. Add tests. --------- Co-authored-by: Jonathan Dekhtiar <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a
PluginLoaderclass that handles loading and querying plugins.This removes some duplicate code, prepares for plugins having more
than one method in the future, and enables using a single cached plugin
list, rather than repeatedly querying and instantiating them.