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 TensorFlow v2.17 compatibility by removing optimizer caching #489

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

Conversation

zeyueN
Copy link
Collaborator

@zeyueN zeyueN commented Sep 12, 2024

User description

Context:

Description of the Change:

Benefits:

Possible Drawbacks:

Related GitHub Issues:


PR Type

bug_fix


Description

  • Removed the caching mechanism for the Euclidean optimizer in backend_manager.py to ensure compatibility with TensorFlow v2.17.
  • The Euclidean optimizer is now always instantiated when accessed, preventing potential issues with stale or incompatible cached objects.

Changes walkthrough 📝

Relevant files
Bug fix
backend_manager.py
Remove caching for Euclidean optimizer to fix compatibility

mrmustard/math/backend_manager.py

  • Removed caching of the Euclidean optimizer object.
  • Always create a new Euclidean optimizer instance.
  • +1/-2     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @codiumai-pr-agent-pro codiumai-pr-agent-pro bot changed the title not use cached optimizer object to make tf v2.17 happy Fix TensorFlow v2.17 compatibility by removing optimizer caching Sep 12, 2024
    Copy link
    Contributor

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🏅 Score: 95
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Performance Concern
    Creating a new optimizer instance on every property access might impact performance if called frequently

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Sep 12, 2024

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Performance
    Initialize the optimizer in the constructor instead of the property getter

    Consider initializing self._euclidean_opt in the class constructor instead of in the
    property getter. This would ensure that the optimizer is created only once when the
    object is instantiated, rather than every time the property is accessed.

    mrmustard/math/backend_manager.py [1316-1320]

    +def __init__(self, *args, **kwargs):
    +    super().__init__(*args, **kwargs)
    +    self._euclidean_opt = self.DefaultEuclideanOptimizer()
    +
     @property
     def euclidean_opt(self):
         r"""The configured Euclidean optimizer."""
    -    self._euclidean_opt = self.DefaultEuclideanOptimizer()
         return self._euclidean_opt
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion improves performance by ensuring the optimizer is created only once during object instantiation, rather than every time the property is accessed. This change aligns with typical object-oriented design practices.

    8
    Best practice
    ✅ Rename property to method if new instance creation is intended

    If creating a new optimizer instance on every access is intentional, consider
    renaming the property to a method (e.g., get_euclidean_opt()) to better reflect its
    behavior of always returning a new instance.

    mrmustard/math/backend_manager.py [1316-1320]

    -@property
    -def euclidean_opt(self):
    -    r"""The configured Euclidean optimizer."""
    -    self._euclidean_opt = self.DefaultEuclideanOptimizer()
    -    return self._euclidean_opt
    +def get_euclidean_opt(self):
    +    r"""Get a new instance of the configured Euclidean optimizer."""
    +    return self.DefaultEuclideanOptimizer()
     

    [Suggestion has been applied]

    Suggestion importance[1-10]: 7

    Why: Renaming the property to a method clarifies the behavior of returning a new instance each time, which improves code readability and aligns with the intended functionality. However, it may not be necessary if the behavior is not intentional.

    7

    Comment on lines 1316 to 1320
    @property
    def euclidean_opt(self):
    r"""The configured Euclidean optimizer."""
    if not self._euclidean_opt:
    self._euclidean_opt = self.DefaultEuclideanOptimizer()
    self._euclidean_opt = self.DefaultEuclideanOptimizer()
    return self._euclidean_opt
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    Suggestion: Rename property to method if new instance creation is intended [Best practice, importance: 7]

    Suggested change
    @property
    def euclidean_opt(self):
    r"""The configured Euclidean optimizer."""
    if not self._euclidean_opt:
    self._euclidean_opt = self.DefaultEuclideanOptimizer()
    self._euclidean_opt = self.DefaultEuclideanOptimizer()
    return self._euclidean_opt
    def get_euclidean_opt(self):
    r"""Get a new instance of the configured Euclidean optimizer."""
    return self.DefaultEuclideanOptimizer()

    @apchytr apchytr added the no changelog Pull request does not require a CHANGELOG entry label Oct 31, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    bug_fix no changelog Pull request does not require a CHANGELOG entry Review effort [1-5]: 2
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants