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] Bump Python minimum version to 3.8 #78828

Merged
merged 2 commits into from
Feb 20, 2024

Conversation

linux4life798
Copy link
Contributor

@linux4life798 linux4life798 commented Jan 20, 2024

As per the RFC https://discourse.llvm.org/t/rfc-upgrading-llvms-minimum-required-python-version/67571,
raise the minimum Python version to ensure that the Python syntax doesn't
become overly obsolete, to enable new Python feature usage, and to improve
the maintainability of CI.

One of the primary use cases for this higher Python version is to enable python type
annotations that are more aligned with current Python best practices. This is not only
important for our own internal Python for testing, but for the Python bindings that
are exposed to users.

Copy link
Contributor

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

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

There are a couple other files that should probably be updated:

  • llvm/docs/GettingStartedVS.rst
  • llvm/docs/TestingGuide.rst

That looks like it should be it when I searched for 3.6 within the repository.

llvm/docs/GettingStarted.rst Outdated Show resolved Hide resolved
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 21, 2024

@llvm/pr-subscribers-testing-tools

Author: Craig Hesling (linux4life798)

Changes

As per the RFC https://discourse.llvm.org/t/rfc-upgrading-llvms-minimum-required-python-version/67571,
raise the minimum Python version to ensure that the Python syntax doesn't
become overly obsolete, to enable new Python feature usage, and ensure the
maintainability of CI.

One of the primary use cases for this higher Python version is to enable python type
annotations that are more aligned with current Python best practices. This is not only
important for our own internal Python for testing, but for the Python bindings that
are exposed to users.


Full diff: https://github.com/llvm/llvm-project/pull/78828.diff

4 Files Affected:

  • (modified) llvm/CMakeLists.txt (+2-2)
  • (modified) llvm/docs/GettingStarted.rst (+3-2)
  • (modified) llvm/docs/GettingStartedVS.rst (+1-1)
  • (modified) llvm/docs/TestingGuide.rst (+1-1)
diff --git a/llvm/CMakeLists.txt b/llvm/CMakeLists.txt
index 1d230004e6c34ec..c7c21971039eec7 100644
--- a/llvm/CMakeLists.txt
+++ b/llvm/CMakeLists.txt
@@ -880,8 +880,8 @@ set(LLVM_PROFDATA_FILE "" CACHE FILEPATH
   "Profiling data file to use when compiling in order to improve runtime performance.")
 
 if(LLVM_INCLUDE_TESTS)
-  # Lit test suite requires at least python 3.6
-  set(LLVM_MINIMUM_PYTHON_VERSION 3.6)
+  # All LLVM Python files should be compatible down to this minimum version.
+  set(LLVM_MINIMUM_PYTHON_VERSION 3.8)
 else()
   # FIXME: it is unknown if this is the actual minimum bound
   set(LLVM_MINIMUM_PYTHON_VERSION 3.0)
diff --git a/llvm/docs/GettingStarted.rst b/llvm/docs/GettingStarted.rst
index da9cc8aea6d32d8..87ae956e3351a0c 100644
--- a/llvm/docs/GettingStarted.rst
+++ b/llvm/docs/GettingStarted.rst
@@ -286,7 +286,7 @@ uses the package and provides other details.
 Package                                                     Version      Notes
 =========================================================== ============ ==========================================
 `CMake <http://cmake.org/>`__                               >=3.20.0     Makefile/workspace generator
-`python <http://www.python.org/>`_                          >=3.6        Automated test suite\ :sup:`1`
+`python <http://www.python.org/>`_                          >=3.8        Automated test suite\ :sup:`1`
 `zlib <http://zlib.net>`_                                   >=1.2.3.4    Compression library\ :sup:`2`
 `GNU Make <http://savannah.gnu.org/projects/make>`_         3.79, 3.79.1 Makefile/build processor\ :sup:`3`
 =========================================================== ============ ==========================================
@@ -294,7 +294,8 @@ Package                                                     Version      Notes
 .. note::
 
    #. Only needed if you want to run the automated test suite in the
-      ``llvm/test`` directory.
+      ``llvm/test`` directory, or if you plan to utilize any auxiliary Python
+      libraries or utilities.
    #. Optional, adds compression / uncompression capabilities to selected LLVM
       tools.
    #. Optional, you can use any other build tool supported by CMake.
diff --git a/llvm/docs/GettingStartedVS.rst b/llvm/docs/GettingStartedVS.rst
index a1eb88dccc9e5c7..4b15272635fbeb9 100644
--- a/llvm/docs/GettingStartedVS.rst
+++ b/llvm/docs/GettingStartedVS.rst
@@ -55,7 +55,7 @@ Visual Studio 2019 so separate installation is not required. If you do install
 CMake separately, Visual Studio 2022 will require CMake Version 3.21 or later.
 
 If you would like to run the LLVM tests you will need `Python
-<http://www.python.org/>`_. Version 3.6 and newer are known to work. You can
+<http://www.python.org/>`_. Version 3.8 and newer are known to work. You can
 install Python with Visual Studio 2019, from the Microsoft store or from
 the `Python web site <http://www.python.org/>`_. We recommend the latter since it
 allows you to adjust installation options.
diff --git a/llvm/docs/TestingGuide.rst b/llvm/docs/TestingGuide.rst
index a692e301fa2c41f..98e6bd31b668791 100644
--- a/llvm/docs/TestingGuide.rst
+++ b/llvm/docs/TestingGuide.rst
@@ -23,7 +23,7 @@ Requirements
 ============
 
 In order to use the LLVM testing infrastructure, you will need all of the
-software required to build LLVM, as well as `Python <http://python.org>`_ 3.6 or
+software required to build LLVM, as well as `Python <http://python.org>`_ 3.8 or
 later.
 
 LLVM Testing Infrastructure Organization

@tru
Copy link
Collaborator

tru commented Jan 22, 2024

This need a release note as well!

Copy link
Member

@jpienaar jpienaar left a comment

Choose a reason for hiding this comment

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

(Biasedly) looks good to me with release notes added.

@linux4life798
Copy link
Contributor Author

linux4life798 commented Jan 23, 2024

This need a release note as well!

I see. So, probably add a note to the Other Changes section of LLVM and to the Python Binding section of clang?

@tru
Copy link
Collaborator

tru commented Jan 23, 2024

I think it needs to go here: https://llvm.org/docs/ReleaseNotes.html#update-on-required-toolchains-to-build-llvm

Since it changes the requirement for building LLVM.

@linux4life798
Copy link
Contributor Author

I think it needs to go here: https://llvm.org/docs/ReleaseNotes.html#update-on-required-toolchains-to-build-llvm

Wow, sorry. I totally missed that section. Sounds good.

@linux4life798
Copy link
Contributor Author

Corrected. Please take another look!

@linux4life798
Copy link
Contributor Author

linux4life798 commented Jan 27, 2024

Looks like we may have hit the first python version issue:

From the Window x64 buildkit job:

-- CTTestTidyModule ignored -- Loadable modules not supported on this platform.
-- LLD version: 19.0.0
-- Performing Test C_SUPPORTS_WERROR_IMPLICIT_FUNCTION_DECLARATION
-- Performing Test C_SUPPORTS_WERROR_IMPLICIT_FUNCTION_DECLARATION - Failed
-- Performing Test C_SUPPORTS_WERROR_MISMATCHED_TAGS
-- Performing Test C_SUPPORTS_WERROR_MISMATCHED_TAGS - Failed
-- Found Python3: C:/Python39/python.exe (found suitable version "3.9.7", minimum required is "3.8") found components: Interpreter Development Development.Module Development.Embed
CMake Error at C:/BuildTools/Common7/IDE/CommonExtensions/Microsoft/CMake/CMake/share/cmake-3.20/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find Python3 (missing: Python3_NumPy_INCLUDE_DIRS NumPy) (found
  suitable version "3.9.7", minimum required is "3.8")
Call Stack (most recent call first):
  C:/BuildTools/Common7/IDE/CommonExtensions/Microsoft/CMake/CMake/share/cmake-3.20/Modules/FindPackageHandleStandardArgs.cmake:594 (_FPHSA_FAILURE_MESSAGE)
  C:/BuildTools/Common7/IDE/CommonExtensions/Microsoft/CMake/CMake/share/cmake-3.20/Modules/FindPython/Support.cmake:3165 (find_package_handle_standard_args)
  C:/BuildTools/Common7/IDE/CommonExtensions/Microsoft/CMake/CMake/share/cmake-3.20/Modules/FindPython3.cmake:485 (include)
  C:/ws/src/mlir/cmake/modules/MLIRDetectPythonEnv.cmake:21 (find_package)
  C:/ws/src/mlir/CMakeLists.txt:172 (mlir_configure_python_dev_packages)
-- Configuring incomplete, errors occurred!
See also "C:/ws/src/build/CMakeFiles/CMakeOutput.log".
See also "C:/ws/src/build/CMakeFiles/CMakeError.log".

It appears to be indicating that the version of Python is fine (3.9.7), but it couldn't find the NumPy pkg.

@boomanaiden154
Copy link
Contributor

boomanaiden154 commented Jan 27, 2024

Seems like that's a MLIR Python dependency?

Numpy gets installed here in the Buildkite CI:
https://github.com/google/llvm-premerge-checks/blob/ff879c8fa1af8f06c8cb2ac7707ceef52965e87e/scripts/steps.py#L136

I wonder if there are multiple versions of Python on the windows runner and the default one isn't the 3.9.7 where the dependencies are getting installed. I'm not sure it's a trivial fix given where it is in the CI VM setup.

@linux4life798
Copy link
Contributor Author

Good find and good point.

@boomanaiden154
Copy link
Contributor

Actually seems to be here: https://github.com/llvm/llvm-project/blob/53fea6fd6f228eb1dbd282b8d076af545dcd8228/.ci/monolithic-windows.sh#L40C64-L41C1

It seems like the configuration and the installation are using the same Python interpreter:

pip install -q -r C:/ws/src/mlir/python/requirements.txt
WARNING: You are using pip version 21.2.3; however, version 23.2.1 is available.
You should consider upgrading via the 'C:\Python39\python.exe -m pip install --upgrade pip' command.

@linux4life798
Copy link
Contributor Author

linux4life798 commented Feb 18, 2024

Very interesting. It looks like the same buildkite Windows x64 job now succeeds. For referencing later, it is buildkite build #37898. I'm going to rebase and push again to see if this buildkite build is flaky.

I couldn't find any obvious changes to files in the .ci/ dir that would have changed this behavior, but it could be the result of some other change.

git log ad95e375b1e35e~2..fb413e08327b ./.ci mlir/python/requirements.txt mlir/CMakeLists.txt

pip install -q -r C:/ws/src/mlir/python/requirements.txt
WARNING: You are using pip version 21.2.3; however, version 23.2.1 is available.
You should consider upgrading via the 'C:\Python39\python.exe -m pip install --upgrade pip' command.

Hmm. I'm not quite sure how the pip upgrade warning would create issues. That's usually a harmless reminder, but I could be wrong.

As per the RFC
https://discourse.llvm.org/t/rfc-upgrading-llvms-minimum-required-python-version/67571,
raise the minimum Python version to ensure that the Python syntax doesn't
become overly obsolete, to enable new Python feature usage, and to improve
the maintainability of CI.
As per the RFC
https://discourse.llvm.org/t/rfc-upgrading-llvms-minimum-required-python-version/67571,
raise the minimum Python version to ensure that the Python syntax doesn't
become overly obsolete, to enable new Python feature usage, and to improve
the maintainability of CI.
@boomanaiden154
Copy link
Contributor

Very interesting. It looks like the same buildkite Windows x64 job now succeeds. For referencing later, it is buildkite build #37898. I'm going to rebase and push again to see if this buildkite build is flaky.

The Windows build (on Buildkite) is disabled currently (more context is available in this PR: #81538 and the discourse thread linked). It's interesting that it started passing. There have been a lot of transient failures on the Windows build that led to failures, so it wouldn't surprise me if this issue ended up being transient.

Given that the CI is disabled, I don't think there's a way to properly test this against the Buildkite Windows CI (well, you might be able to revert the patch from the disable commit in this branch), but that also shouldn't matter if it's disabled. I'm fine with this landing in its current state (especially if it passed the Windows CI before you rebased). The bigger issue either way is probably making sure everything gets past post-commit.

Some stuff within the project is also already on a newer version (#81598), so this landing sooner rather than later would probably be good.

Given that the RFC has consensus and the Windows CI failure is transient, I think we can probably move forward with this. Does anyone else have objections?

@linux4life798
Copy link
Contributor Author

linux4life798 commented Feb 18, 2024

Much agreed.

@jyknight
Copy link
Member

SGTM. Watch out for post submit bot failures.

@boomanaiden154
Copy link
Contributor

Alright. I'm going to merge this tonight (@linux4life798 you should probably apply for commit access soonish too now that you've landed a couple commits) in the evening when buildbot traffic is hopefully low. I'll keep an eye on the post-commit bots and see what ends up failing. I'd almost expect failures, so wouldn't be surprised if it gets reverted initially, but that will let us know what breaks and we can move from there.

@boomanaiden154 boomanaiden154 merged commit 0a6c74e into llvm:main Feb 20, 2024
5 checks passed
boomanaiden154 added a commit that referenced this pull request Feb 20, 2024
This reverts commit 0a6c74e.

This created a lot of post-commit failures due to buildbots running
older versions of Python.
@boomanaiden154
Copy link
Contributor

Reverted. This created a lot of post-commit failures. We'll have to get the buildbot maintainers to upgrade the Python version on those bots if we want to get this through.

https://lab.llvm.org/buildbot/#/changes/124772

@DeinAlptraum
Copy link
Contributor

Is there anything happening here rn? @linux4life798 do you need any help with this?

@boomanaiden154
Copy link
Contributor

Getting buildbots updated and relanding the change and potentially repeating that process is the next step. I've opened #83962 and emailed buildbot maintainers for bots that failed after landing this change due to old Python versions.

boomanaiden154 added a commit that referenced this pull request Apr 20, 2024
This reverts commit 2dfa30d.

This relands commit 0a6c74e.

This patch originally caused a host of buildbot failures due to several
buildbots not having the necessary python version. That was tracked in
issue #83962, and all bots that failed in the first round have now been
updated. This is an attempt to reland the patch to see if it sticks or
if there are a number of long-running bots where this patch will cause
failures.
boomanaiden154 added a commit that referenced this pull request Apr 20, 2024
This reverts commit f293118.

This was again causing buildbot failures. #83962 has been updated with the new
failures, notifying the buildbot maintainers that they need to update their
bots.
aniplcc pushed a commit to aniplcc/llvm-project that referenced this pull request Apr 21, 2024
This reverts commit 2dfa30d.

This relands commit 0a6c74e.

This patch originally caused a host of buildbot failures due to several
buildbots not having the necessary python version. That was tracked in
issue llvm#83962, and all bots that failed in the first round have now been
updated. This is an attempt to reland the patch to see if it sticks or
if there are a number of long-running bots where this patch will cause
failures.
aniplcc pushed a commit to aniplcc/llvm-project that referenced this pull request Apr 21, 2024
…)""

This reverts commit f293118.

This was again causing buildbot failures. llvm#83962 has been updated with the new
failures, notifying the buildbot maintainers that they need to update their
bots.
boomanaiden154 added a commit that referenced this pull request Jun 7, 2024
This reverts commit b6824c9.

This relands commit 0a6c74e.

The original commit was reverted due to buildbot failures. These bots
should be updated now, so hopefully this will stick.
HendrikHuebner pushed a commit to HendrikHuebner/llvm-project that referenced this pull request Jun 8, 2024
This reverts commit b6824c9.

This relands commit 0a6c74e.

The original commit was reverted due to buildbot failures. These bots
should be updated now, so hopefully this will stick.
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Jun 9, 2024
…)""

This reverts commit 33f4a77.

Change-Id: Icd0d5f9a1fe3fbbdbbc320240ba06f36c4b66a37
nekoshirro pushed a commit to nekoshirro/Alchemist-LLVM that referenced this pull request Jun 9, 2024
This reverts commit b6824c9.

This relands commit 0a6c74e.

The original commit was reverted due to buildbot failures. These bots
should be updated now, so hopefully this will stick.

Signed-off-by: Hafidz Muzakky <ais.muzakky@gmail.com>
@HerrCai0907 HerrCai0907 mentioned this pull request Jun 13, 2024
chapuni added a commit to chapuni/llvm-project that referenced this pull request Jun 13, 2024
…)""

This reverts commit 33f4a77.
llvmorg-19-init-13732-g33f4a77d9218
chapuni added a commit to chapuni/llvm-project that referenced this pull request Jun 13, 2024
…)""

This reverts commit 33f4a77.
llvmorg-19-init-13732-g33f4a77d9218
chapuni added a commit to chapuni/llvm-project that referenced this pull request Jun 13, 2024
…)""

This reverts commit 33f4a77.
llvmorg-19-init-13732-g33f4a77d9218
chapuni added a commit to chapuni/llvm-project that referenced this pull request Jun 14, 2024
…)""

This reverts commit 33f4a77.
llvmorg-19-init-13732-g33f4a77d9218
chapuni added a commit to chapuni/llvm-project that referenced this pull request Jun 14, 2024
…)""

This reverts commit 33f4a77.
llvmorg-19-init-13732-g33f4a77d9218
chapuni added a commit to chapuni/llvm-project that referenced this pull request Jun 14, 2024
…)""

This reverts commit 33f4a77.
llvmorg-19-init-13732-g33f4a77d9218
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants