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

[ENH] make get_packages_with_changed_specs safe to mutation of return #6451

Merged
merged 1 commit into from
May 22, 2024

Conversation

fkiraly
Copy link
Collaborator

@fkiraly fkiraly commented May 19, 2024

If an lru_cache-d function returns a mutable object, and that object is later muted, the cached function wlil continue returning the muted - incorrect - return.

This PR fixes the one instance in the code base where this issue could arise.

@fkiraly fkiraly added module:tests test framework functionality - only framework, excl specific tests enhancement Adding new functionality labels May 19, 2024
fkiraly added a commit to sktime/skpro that referenced this pull request May 20, 2024
…rn (#348)

If an `lru_cache`-d function returns a mutable object, and that object
is later muted, the cached function wlil continue returning the muted -
incorrect - return.

This PR fixes the one instance in the code base where this issue could
arise.

Mirror of sktime/sktime#6451
Copy link
Collaborator

@yarnabrina yarnabrina left a comment

Choose a reason for hiding this comment

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

(Not a review, because I've very limited experience with lru_cache method.)

Can you please explain what is the current issue, and in which scenario it has an adverse effect?

@fkiraly
Copy link
Collaborator Author

fkiraly commented May 21, 2024

Sure. Let's suppose we have the following method that returns lists of length n (but imagine it is a much more tedious, resource consuming operation that happens internally).

def compute_sequence(n):
    return list(range(n))   # imagine the logic is more resource intensive and tedious

If we now lru_cache the method compute_sequence, and do the following:

my_sequence = compute_sequence(42)
my_sequence[3] = 4242

then every subsequent call compute_sequence(42) will return the changed list, until it is mutated again.

This is admittedly a low risk for the function in this PR, given that it is used only internally, but from the above I derive that no lru_cache-d method should have a mutable return.

@yarnabrina
Copy link
Collaborator

Understood, thanks for the explanation.

@fkiraly fkiraly merged commit 1986820 into main May 22, 2024
53 checks passed
@fkiraly fkiraly deleted the safe-lru_cache branch May 22, 2024 18:49
geetu040 pushed a commit to geetu040/sktime that referenced this pull request Jun 4, 2024
…rn (sktime#6451)

If an `lru_cache`-d function returns a mutable object, and that object
is later muted, the cached function wlil continue returning the muted -
incorrect - return.

This PR fixes the one instance in the code base where this issue could
arise.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adding new functionality module:tests test framework functionality - only framework, excl specific tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants