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

Python 3.12 version bump #528

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

Python 3.12 version bump #528

wants to merge 17 commits into from

Conversation

apchytr
Copy link
Collaborator

@apchytr apchytr commented Nov 27, 2024

User description

Context: Support for python 3.12. This includes bumping the tensorflow version.

Description of the Change: Updated the pyproject.toml to support python 3.12 and bumped the tensorflow version to min 2.16.0. Removed seemingly unnecessary dependencies. Removed macOS warning about switching to tensorflow 2.15.0.

Note: test_training and test_opt fail with tf > 2.15.0. For now these tests are skipped and are to be fixed in the future.


PR Type

enhancement, dependencies


Description

  • Added support for Python 3.12 by updating the classifiers and python version range in pyproject.toml.
  • Bumped the TensorFlow version to 2.18.0 to ensure compatibility with Python 3.12.
  • Simplified TensorFlow dependencies by removing platform-specific configurations, likely due to improved compatibility in newer TensorFlow releases.

Changes walkthrough 📝

Relevant files
Dependencies
pyproject.toml
Update Python and TensorFlow version support in pyproject.toml

pyproject.toml

  • Added support for Python 3.12 in classifiers.
  • Updated Python version range to include 3.12.
  • Bumped TensorFlow version to 2.18.0.
  • Removed detailed TensorFlow platform-specific configurations.
  • +3/-22   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @apchytr apchytr added WIP work in progress no changelog Pull request does not require a CHANGELOG entry labels Nov 27, 2024
    @codiumai-pr-agent-pro codiumai-pr-agent-pro bot added enhancement New feature or request dependencies Pull requests that update a dependency file labels Nov 27, 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 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🏅 Score: 85
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Compatibility Risk
    The jump to TensorFlow 2.18.0 is significant and may introduce breaking changes. Verify that all TensorFlow-dependent functionality works as expected with the new version.

    Platform Support
    Removal of platform-specific TensorFlow configurations may affect compatibility on certain platforms like macOS ARM or Windows. Validate the package installation and functionality across different platforms.

    pyproject.toml Outdated Show resolved Hide resolved
    pyproject.toml Outdated Show resolved Hide resolved
    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Nov 27, 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 ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    ✅ Specify an existing and stable version of a dependency instead of a future, non-existent one
    Suggestion Impact:The suggestion led to changing the TensorFlow version from a non-existent 2.18.0 to a stable version 2.15.0, with an additional constraint to be less than 2.19.0.

    code diff:

    -tensorflow = {version = "^2.18.0" }
    +tensorflow = {version = "^2.15.0,<2.19.0" }

    TensorFlow 2.18.0 does not exist yet (latest is 2.15.0). Using a non-existent
    version will cause installation failures. Revert to the latest stable version
    2.15.0.

    pyproject.toml [47]

    -tensorflow = {version = "^2.18.0" }
    +tensorflow = {version = "^2.15.0" }
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: Using a non-existent TensorFlow version (2.18.0) would completely break package installation. This is a critical issue that needs to be fixed to maintain project functionality.

    10
    Maintain platform-specific dependency configurations to ensure compatibility across different operating systems and architectures

    Removing platform-specific TensorFlow configurations may cause installation issues
    on certain platforms like macOS M1/M2 (arm64) which require tensorflow-macos.
    Consider keeping platform-specific dependencies.

    pyproject.toml [47]

    -tensorflow = {version = "^2.18.0" }
    +tensorflow = {version = "^2.15.0" }
    +tensorflow-macos = { version = "2.15.0", platform = "darwin", markers = "platform_machine=='arm64'" }
    +tensorflow-intel = { version = "^2.15.0", platform = "win32" }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Removing platform-specific TensorFlow configurations will break installation on specific platforms like macOS M1/M2 and Windows, affecting a significant portion of users. This is a serious compatibility issue.

    9

    💡 Need additional feedback ? start a PR chat

    Copy link

    codecov bot commented Nov 28, 2024

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 89.48%. Comparing base (4c603d0) to head (07b48b9).

    Additional details and impacted files
    @@             Coverage Diff             @@
    ##           develop     #528      +/-   ##
    ===========================================
    - Coverage    89.84%   89.48%   -0.37%     
    ===========================================
      Files           93       93              
      Lines         6098     6094       -4     
    ===========================================
    - Hits          5479     5453      -26     
    - Misses         619      641      +22     
    Files with missing lines Coverage Δ
    mrmustard/math/backend_tensorflow.py 100.00% <ø> (ø)

    ... and 5 files with indirect coverage changes


    Continue to review full report in Codecov by Sentry.

    Legend - Click here to learn more
    Δ = absolute <relative> (impact), ø = not affected, ? = missing data
    Powered by Codecov. Last update 4c603d0...07b48b9. Read the comment docs.

    @apchytr apchytr removed the WIP work in progress label Nov 28, 2024
    @@ -413,15 +409,7 @@ def sqrtm(self, tensor: tf.Tensor, dtype, rtol=1e-05, atol=1e-08) -> Tensor:
    # ~~~~~~~~~~~~~~~~~

    def DefaultEuclideanOptimizer(self) -> tf.keras.optimizers.legacy.Optimizer:
    use_legacy = Version(metadata.distribution("tensorflow").version) < Version("2.16.0")
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    i know we drop 2.15 in this PR, but do we happen to know if the Adam optimizer is still slow on MacOS? I can't find any relevant issues or documentation.

    I'm ok to just say this will be slow on Mac users for the time being if this upgrade is fairly critical. I definitely like how much easier installation is.

    Copy link
    Collaborator Author

    @apchytr apchytr Dec 2, 2024

    Choose a reason for hiding this comment

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

    Based on running the tests that work on >=2.15.0 (meaning actual optimization tests are skipped) they are roughly the same. 0:03:02 for 2.15.0 and 0:03:09 for 2.18.0

    Copy link
    Collaborator Author

    Choose a reason for hiding this comment

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

    That is to say, preliminary profiling suggests they're similar but we really wouldn't know until the optimization itself is fixed. Considering tensorflow-macos is no longer the suggested installation for macos I think it should be fine

    Skips tests if the backend is tensorflow and the version is ``>2.15.0``.
    """
    if math.backend_name == "tensorflow":
    if Version(metadata.distribution("tensorflow").version) > Version("2.15.0"):
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    it always is now, right?

    Copy link
    Collaborator Author

    Choose a reason for hiding this comment

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

    Right now yes. I'm still debating on having 2.15.0 be the lowest but then you have to jump through some hoops to have tensorflow-macos install when tf == 2.15.0 but not install if tf>2.15.0.

    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    hmm. we haven't released since March - @elib20 is there a plan to make another official MrMustard version? I'm wondering how bad it would be to just say "if using MrMustard 7.4+, then you need TF 2.16+"

    Copy link
    Collaborator Author

    Choose a reason for hiding this comment

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

    I think the next planned release is once lab dev is complete. Seeing as that would be a massive update from the previous release it's completely fine to mention in the CHANGELOG that we just bumped the version

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    dependencies Pull requests that update a dependency file enhancement New feature or request 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.

    4 participants