Skip to content

fix: respect replace=True for already-registered custom resolvers - #5828

Merged
merelcht merged 4 commits into
kedro-org:mainfrom
Shahzeb-DataEngineer:fix/respect-replace-for-registered-resolvers
Nov 5, 2026
Merged

fix: respect replace=True for already-registered custom resolvers#5828
merelcht merged 4 commits into
kedro-org:mainfrom
Shahzeb-DataEngineer:fix/respect-replace-for-registered-resolvers

Conversation

@Shahzeb-DataEngineer

@Shahzeb-DataEngineer Shahzeb-DataEngineer commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

Description

Since #5813, a custom resolver can be given as a (callable, kwargs) tuple so kwargs like use_cache or replace reach OmegaConf.register_new_resolver(). But OmegaConfigLoader._register_new_resolvers skipped every resolver whose name was already registered (if not OmegaConf.has_resolver(name)), so the kwargs never got passed on and replace=True silently did nothing: replacing a resolver only matters when one with that name already exists, which is exactly the skipped case.

This PR changes the guard so an already-registered resolver is skipped only when its kwargs do not contain replace=True; with replace=True it is registered again with the given kwargs. Plain callables and tuples without replace keep their previous behaviour (an extra debug log now explains why a resolver was skipped).

Also adds a RELEASE.md entry and two regression tests in tests/config/test_omegaconf_config.py: one asserting replace=True swaps the implementation (new world), one asserting the old implementation is kept without replace (old world).

Related Issue

closes #5825

@Shahzeb-DataEngineer
fix: respect replace=True for already-registered custom resolvers
964e037 
_register_new_resolvers skipped every resolver whose name was already
registered, so kwargs such as replace=True never reached
OmegaConf.register_new_resolver() and the old resolver was kept silently.
An already-registered resolver is now skipped only when the resolver
kwargs do not contain replace=True; plain callables and tuples without
replace keep their previous behaviour.

Signed-off-by: Shahzeb Ali <[email protected]>
Copilot AI balanced review requested due to automatic review settings October 20, 2026 05:26
Copilot AI reviewed Oct 20, 2026

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

@jitu5

jitu5 commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

Thanks @Shahzeb-DataEngineer for this PR! The issue associated with this PR has not been marked for external contribution yet. As a policy, we only accept contributions for issues marked as "Ready" on this board: OS contribution tasks
I'll mark this PR as draft and bring the ticket through internal discussion.

@jitu5
jitu5 marked this pull request as draft October 20, 2026 13:18
@Shahzeb-DataEngineer

Shahzeb-DataEngineer commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @jitu5, understood. No rush at all, happy to wait while the ticket goes through internal discussion. The PR is ready from my side whenever the issue is marked Ready; just let me know if you would like any changes in the meantime.

@Shahzeb-DataEngineer Shahzeb-DataEngineer mentioned this pull request Oct 23, 2026
Closed
@merelcht
merelcht marked this pull request as ready for review November 2, 2026 15:23
@merelcht
merelcht requested a review from ankatiyar November 2, 2026 15:23
merelcht
merelcht reviewed Nov 2, 2026
View reviewed changes
Comment thread RELEASE.md Resolved
Comment thread kedro/config/omegaconf_config.py Resolved
Comment thread tests/config/test_omegaconf_config.py Resolved
@Shahzeb-DataEngineer
fix: address review feedback on replace for registered resolvers
41ea509 
Move the RELEASE.md note from Bug fixes into the kedro-org#5813 entry under
Major features and improvements, since kedro-org#5813 is unreleased; document
the skip-unless-replace rule in the OmegaConfigLoader docstrings and
in the templating guide; and clear the test resolvers after each
test so the tests stay valid when re-run in the same process.

Signed-off-by: Shahzeb Ali <[email protected]>
@Shahzeb-DataEngineer

Shahzeb-DataEngineer commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @merelcht, all three points are addressed in the latest commit:

  • The bug-fix RELEASE.md entry is dropped; the skip-unless-replace sentence is now part of the existing feat(config): pass kwargs to custom resolvers registered with OmegaConfigLoader #5813 entry under Major features and improvements.
  • The skip rule is documented in the _register_new_resolvers docstring, in the custom_resolvers argument docs, and in docs/configure/how_to_use_templating.md (with the %reload_kedro / multiple-loaders case and a replace=True example).
  • The two new tests now use a yield fixture that clears replaceable_greet / keepable_greet from OmegaConf's registry after each test, so they stay valid on re-runs in the same process.

Tests: the custom_resolvers tests pass, including both new tests run twice in one process.

merelcht and others added 2 commits November 3, 2026 12:39
merelcht
merelcht approved these changes Nov 3, 2026
View reviewed changes

@merelcht merelcht left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @Shahzeb-DataEngineer, this looks good to me now! ⭐

@Galileo-Galilei Galileo-Galilei mentioned this pull request Nov 4, 2026
Open
7 tasks done
@merelcht
merelcht removed the request for review from AliceCima10 November 5, 2026 12:14
@merelcht
merelcht merged commit 7acdcae into kedro-org:main Nov 5, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@merelcht merelcht merelcht approved these changes
@ankatiyar ankatiyar ankatiyar approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

OmegaConfigLoader ignores replace=True for a custom resolver that is already registered

5 participants

@Shahzeb-DataEngineer @jitu5 @merelcht @ankatiyar