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

Unwrap functions when extracting global variable references #2604

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

mwaskom
Copy link
Contributor

@mwaskom mwaskom commented Dec 3, 2024

Fixes CLI-276

Changelog

  • Image build functions that use a functools.wraps decorator will now have their global variables included in the cache key. Previously, the cache would use global variables referenced within the wrapper itself. This will force a rebuild for Image layers defined using wrapped functions.

Copy link
Contributor

@aksh-at aksh-at left a comment

Choose a reason for hiding this comment

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

Nice, thanks for tracking this down! Left an evil comment.

@@ -249,6 +249,12 @@ def get_globals(self) -> dict[str, Any]:
from .._vendor.cloudpickle import _extract_code_globals

func = self.raw_f
while True:
# Unwrap functions decorated using functools.wrapped (potentially multiple times)
try:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a really contrived example and I hope no one will do this:

import functools


def f():
    print("hi")


functools.wraps(f)(f)

print(f.__wrapped__ is f) # True
print(f.__wrapped__.__wrapped__ is f) # True

But just in case, we could guard against func != func.__wrapped__ so we don't go into an infinite loop?

@mwaskom mwaskom force-pushed the michael/2024-12-03-handle-wrapped-build-functions branch from 643839b to 7c12cf4 Compare December 4, 2024 01:47
@mwaskom mwaskom force-pushed the michael/2024-12-03-handle-wrapped-build-functions branch from 7c12cf4 to 9b05fea Compare December 4, 2024 01:50
@mwaskom
Copy link
Contributor Author

mwaskom commented Dec 4, 2024

@prbot automerge

@modal-pr-review-automation
Copy link

Some required workflows are pending. Will automerge once they complete successfully.

@modal-pr-review-automation
Copy link

Cannot automerge PR because workflow 'CI / CD' failed.

@mwaskom mwaskom merged commit 638c087 into main Dec 4, 2024
22 checks passed
@mwaskom mwaskom deleted the michael/2024-12-03-handle-wrapped-build-functions branch December 4, 2024 02:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants