Skip to content

[master] Matchers can self-reference the loader - #64607

Open
frebib wants to merge 3 commits into
saltstack:masterfrom
frebib:frebib/61950-alt
Open

[master] Matchers can self-reference the loader#64607
frebib wants to merge 3 commits into
saltstack:masterfrom
frebib:frebib/61950-alt

Conversation

@frebib

@frebib frebib commented Jul 7, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

This makes it so a loaded matcher doesn't have to load another instance of the loader itself and can instead reuse the existing matchers that are already loaded. This should speed up many matcher operations considerably.

This is an alternative approach to #64606 which performs the same but is much much cleaner

What does this PR do?

What issues does this PR fix or reference?

Fixes: #61950

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes/No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@frebib
frebib requested a review from a team as a code owner July 7, 2023 00:50
@frebib
frebib requested review from garethgreenaway and removed request for a team July 7, 2023 00:50
@frebib

frebib commented Jul 7, 2023

Copy link
Copy Markdown
Contributor Author

Okay yeah this works and it drops our pillar topfile matching from ~4-5s to <200ms

@frebib
frebib temporarily deployed to ci July 7, 2023 01:04 — with GitHub Actions Inactive
@frebib
frebib temporarily deployed to ci July 7, 2023 01:04 — with GitHub Actions Inactive
@frebib
frebib temporarily deployed to ci July 7, 2023 01:04 — with GitHub Actions Inactive
@frebib
frebib temporarily deployed to ci July 7, 2023 01:04 — with GitHub Actions Inactive
@frebib
frebib temporarily deployed to ci July 7, 2023 01:19 — with GitHub Actions Inactive
@frebib
frebib temporarily deployed to ci July 7, 2023 01:26 — with GitHub Actions Inactive
@frebib
frebib temporarily deployed to ci July 7, 2023 02:20 — with GitHub Actions Inactive
@frebib
frebib temporarily deployed to ci July 7, 2023 02:20 — with GitHub Actions Inactive
@frebib
frebib temporarily deployed to ci July 7, 2023 02:20 — with GitHub Actions Inactive
@frebib
frebib temporarily deployed to ci July 7, 2023 02:20 — with GitHub Actions Inactive
@frebib
frebib temporarily deployed to ci July 7, 2023 02:20 — with GitHub Actions Inactive
@frebib
frebib temporarily deployed to ci July 7, 2023 02:20 — with GitHub Actions Inactive
@frebib
frebib temporarily deployed to ci July 7, 2023 02:57 — with GitHub Actions Inactive
@frebib
frebib temporarily deployed to ci July 7, 2023 02:57 — with GitHub Actions Inactive
@frebib
frebib temporarily deployed to ci July 7, 2023 02:57 — with GitHub Actions Inactive
@frebib
frebib temporarily deployed to ci July 7, 2023 02:57 — with GitHub Actions Inactive
@frebib
frebib temporarily deployed to ci July 7, 2023 02:57 — with GitHub Actions Inactive
@frebib
frebib temporarily deployed to ci July 7, 2023 02:57 — with GitHub Actions Inactive
@s0undt3ch

s0undt3ch commented Jul 8, 2023

Copy link
Copy Markdown
Contributor

Can you please address the pre-commit issues(https://docs.saltproject.io/en/latest/topics/development/contributing.html#set-up-pre-commit-and-nox), and, for lint, please also add __matchers__ to the .pylintrc file under additional-builtins.

Thanks!

@s0undt3ch
s0undt3ch requested a review from dwoz July 8, 2023 20:13
@frebib

frebib commented Jul 9, 2023

Copy link
Copy Markdown
Contributor Author

@s0undt3ch This also requires a change to pytest-salt-factories because that hardcodes a list of dunders. We might also need to add this new dunder to a few of the other loader functions too, like minion_mods(). Starting with pytest-salt-factories, should I send a PR there first? It'll be required to make the tests pass

@frebib

frebib commented Jul 9, 2023

Copy link
Copy Markdown
Contributor Author

fyi half of the pre-commit hooks fail on python3.11 because you're pinning ancient library versions that don't support 3.11, mostly because they're trying to import formatargspec and other deprecated functions

  File "/home/frebib/.cache/pre-commit/repoe0j2g1y8/py_env-python3/lib/python3.11/site-packages/wrapt/__init__.py", line 10, in <module>
    from .decorators import (adapter_factory, AdapterFactory, decorator,
  File "/home/frebib/.cache/pre-commit/repoe0j2g1y8/py_env-python3/lib/python3.11/site-packages/wrapt/decorators.py", line 34, in <module>
    from inspect import ismethod, isclass, formatargspec
ImportError: cannot import name 'formatargspec' from 'inspect' (/usr/lib/python3.11/inspect.py)
nox > Command pylint --rcfile=.pylintrc --disable=I tests/support/pytest/loader.py failed with exit code 1

@frebib
frebib force-pushed the frebib/61950-alt branch 2 times, most recently from 468b8ad to b96c838 Compare July 9, 2023 12:49
frebib
frebib commented Jul 9, 2023
View reviewed changes
Comment thread salt/loader/__init__.py
if ext_dirs:
if ext_type_dirs is None:
ext_type_dirs = "{}_dirs".format(tag)
ext_type_dirs = f"{tag}_dirs"

@frebib frebib Jul 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annoyingly pre-commit has made a mess of this patch including unrelated changes.
It would be better to pyupgrade/black/isort the whole repo in a single commit so contributor patches can be as clean as possible

@dwoz dwoz Oct 5, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this could be done in a bulk or several smaller PRs. That said, for now you could also separate the your changes from pre-commit's in two separate commits. I think --no-verify can accomplish that.

frebib added a commit to frebib/pytest-salt-factories that referenced this pull request Jul 9, 2023
@frebib
Add __matchers__ to default dunders
f7fa463 
Required for saltstack/salt#64607

Signed-off-by: Joe Groocock <jgroocock@cloudflare.com>
dwoz
dwoz requested changes Oct 5, 2023
View reviewed changes

@dwoz dwoz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with this when it has more tests. 👍

@dwoz dwoz added this to the Argon v3008.0 milestone Dec 18, 2023
@dwoz
dwoz requested a review from a team as a code owner March 16, 2025 22:09
twangboy
twangboy requested changes Mar 31, 2025
View reviewed changes

@twangboy twangboy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a few tests. And a rebase.

twangboy
twangboy requested changes Jul 3, 2025
View reviewed changes

@twangboy twangboy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create a changelog for this

@twangboy twangboy added needs-changelog test:full Run the full test suite needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases labels Jul 3, 2025
@twangboy twangboy added the merge-conflict PR has a merge conflict label Aug 6, 2025
@twangboy twangboy modified the milestones: Argon v3008.0, Argon v3008.1 Jun 5, 2026
@dwoz
dwoz force-pushed the frebib/61950-alt branch from b96c838 to 9bfe5ba Compare June 16, 2026 00:42
@dwoz dwoz modified the milestones: Argon v3008.1, Potassium v3009.0 Jun 16, 2026
dwoz added a commit to frebib/salt that referenced this pull request Jun 16, 2026
@dwoz
Add changelog and expand tests for matchers self-reference loader
9bfe5ba 
Add changelog entry for PR saltstack#64607 and replace the now-obsolete
test_matchers_from_context test (which tested __context__ caching) with
tests that verify __matchers__ is injected and never causes recursive
salt.loader.matchers() calls.
@dwoz dwoz removed merge-conflict PR has a merge conflict needs-changelog labels Jun 16, 2026
@dwoz dwoz removed the needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Jun 16, 2026
frebib and others added 3 commits July 2, 2026 16:04
@frebib @twangboy
Matchers can self-reference the loader
37cdbaf 
This makes it so a loaded matcher doesn't have to load another instance
of the loader itself and can instead reuse the existing matchers that
are already loaded. This should speed up many matcher operations
considerably.

Signed-off-by: Joe Groocock <jgroocock@cloudflare.com>
@dwoz @twangboy
Add changelog and expand tests for matchers self-reference loader
7f07b57 
Add changelog entry for PR saltstack#64607 and replace the now-obsolete
test_matchers_from_context test (which tested __context__ caching) with
tests that verify __matchers__ is injected and never causes recursive
salt.loader.matchers() calls.
@dwoz @twangboy
Fix black formatting in compound_match.py
47ea368
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dwoz dwoz dwoz requested changes
@twangboy twangboy twangboy approved these changes
@garethgreenaway garethgreenaway Awaiting requested review from garethgreenaway garethgreenaway was automatically assigned from saltstack/team-core

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

test:full Run the full test suite

Projects

None yet

Milestone

Potassium v3009.0

Development

Successfully merging this pull request may close these issues.

top_matches slows down with every match and takes a lot of time with big pillars (slow deepcopy)

4 participants

@frebib @s0undt3ch @dwoz @twangboy