Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix instantiate side effect modifying input config parent #3005

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jesszzzz
Copy link
Contributor

@jesszzzz jesszzzz commented Jan 6, 2025

Motivation

See #3001 for details, tldr if you call hydra.utils.instantiate on a subconfig, it might modify the parent config through omegaconf resolution which is an unexpected side effect. While there is currently logic to deepcopy the subconfig to avoid such a side effect, we actually need to deepcopy the entire config to avoid it.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

New and existing tests pass

Related Issues and PRs

(Is this PR part of a group of changes? Link the other relevant PRs and Issues here. Use https://help.github.com/en/articles/closing-issues-using-keywords for help on GitHub syntax)

Fixes #3001

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 6, 2025
@andywag
Copy link

andywag commented Jan 7, 2025

I've been looking in to ways to remove these type of copy operations because they have a huge impact on performance.

Is this copy done only at the top level or is it done recursively?
When does this occur? Only when the config is resolved (ie string interpolation)?
How big of a problem is this?

We should probably check the performance hit for key users before landing it.

@jesszzzz
Copy link
Contributor Author

jesszzzz commented Jan 7, 2025

Agree it's not ideal for perf but not sure how to get around it given OmegaConf.resolve modifies the config in place

Is this copy done only at the top level or is it done recursively?
The copy only happens at the top level so it should happen just once per user call to instantiate
When does this occur? Only when the config is resolved (ie string interpolation)? How big of a problem is this?
It only happens when the hydra.utils.instantiate function is called, so it's specific to using _target_. It only happens in specific cases so not that common, but very confusing when it happens. Maybe I can look into only doing the deep copy when it's necessary or add a flag to disable it at the user's risk?

We should probably check the performance hit for key users before landing it.
Makes sense, will look more into which key users use this and when!

Copy link

@andywag andywag left a comment

Choose a reason for hiding this comment

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

I'm slightly concerned about the perf but if it only happens once it shouldn't be a big problem.

@jesszzzz jesszzzz force-pushed the fix_instantiate_side_effect branch from 1042e9a to 65401f5 Compare January 7, 2025 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Instantiate resolves parent config as side effect
3 participants