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 11 and 12 support #2441

Merged
merged 25 commits into from
Dec 9, 2024
Merged

Python 11 and 12 support #2441

merged 25 commits into from
Dec 9, 2024

Conversation

edwardalee
Copy link
Collaborator

@edwardalee edwardalee commented Nov 25, 2024

This PR makes Python versions 3.11, 3.12, and 3.13, at least, work with the Python target.
By default, the Python version is found by CMake with the constraint that it be at least 3.10.0.
This is specified by the following directive:

            find_package(Python 3.10.0 REQUIRED COMPONENTS Interpreter Development)

Because CMake does not work as advertised (see below), this PR adds a target parameter python-version to specify a precise version to use. For example, to use 3.13.0, you would specify:

target Python {
  python-version: "3.13.0"
}

This will change the above directive in the generated CMakeLists.txt file to read like this:

            find_package(Python 3.13.0 EXACT REQUIRED COMPONENTS Interpreter Development)

This PR fixes some memory issues, particularly with modal models, that were previously causing crashes with Python versions greater than 3.10.


The rest of this text documents the issue with CMake.

There is a problem I need help with, however. In this PR, the Python version is hardwired to 3.12.7 in the PythonGenerator.java file, which generates the CMake directives, and has this line:

            find_package(Python 3.12.7...<3.13.0 REQUIRED COMPONENTS Interpreter Development)

The problem is that if this line allows 3.10.0 up to 3.13.0, which is what we want, then lfc inevitably picks a different Python from one identified by CMake, and you get an error message like:

SystemError: initialization of LinguaFrancaActionDelay did not return an extension module

How can we force CMake to pick the Python version that is in the PATH? Sadly, it does not do that.
This supersedes #1902, which I will close.

@cmnrd
Copy link
Collaborator

cmnrd commented Nov 27, 2024

It is not sufficient to modify your PATH to point to a different python version. Cmake does respect both venv and pyenv, and probably also other ways of managing Python environments.

@edwardalee
Copy link
Collaborator Author

edwardalee commented Nov 27, 2024

Unfortunately, it does not seem to be the case the cmake respects pyenv. E.g., I have this:

 % pyenv versions
  system
* 3.10.13 (set by /Users/edwardlee/.pyenv/version)
  3.11.5
  3.12.0
  3.12.7

Nevertheless, cmake uses Python 3.12.7:

% cd ~/LinguaFranca/test/Python
% lfc-dev src/ActionDelay.lf
...
--- Executing command: cmake -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=/Users/edwardlee/LinguaFranca/test/Python -DCMAKE_INSTALL_BINDIR=bin -DLF_FILE_SEPARATOR='"/"' -DLF_SOURCE_DIRECTORY='"/Users/edwardlee/LinguaFranca/test/Python/src"' -DLF_PACKAGE_DIRECTORY='"/Users/edwardlee/LinguaFranca/test/Python"' -DLF_SOURCE_GEN_DIRECTORY='"/Users/edwardlee/LinguaFranca/test/Python/src-gen/ActionDelay"' /Users/edwardlee/LinguaFranca/test/Python/src-gen/ActionDelay
...
-- Found Python: /opt/homebrew/Frameworks/Python.framework/Versions/3.12/bin/python3.12 (found suitable version "3.12.7", required range is "3.10.0...<3.13.0") found components: Interpreter Development Development.Module Development.Embed

The same occurs if I run cmake manually in the src-gen directory, so this is not an lfc problem. So, empirical evidence shows that cmake does NOT respect pyenv. Note that I'm running the latest cmake:

% cmake --version
cmake version 3.31.1

The older version of cmake before updating to this version exhibits the same behavior.

lfc generates the following line in the CMakeLists.txt file:

            find_package(Python 3.10.0...<3.13.0 REQUIRED COMPONENTS Interpreter Development)

My plan is to modify lfc to constrain find_package to exactly the version the path, which should then work with pyenv.

@cmnrd
Copy link
Collaborator

cmnrd commented Nov 28, 2024

Did you have a look at the hints for ind_python? Python_FIND_STRATEGY should default to LOCATION. You can double check its value to make sure that cmake does not try to go for the highest version it can find. Also, relevant are Python_FIND_VIRTUALENV and Python_FIND_FRAMEWORK which you could try to set to FIRST (or ONLY) and LAST respectively.

@edwardalee
Copy link
Collaborator Author

edwardalee commented Nov 28, 2024

Thanks for the pointer to HINTS! I tried the following, but it still fails to find the version of Python specified by pyenv:

            set(Python_FIND_VIRTUALENV FIRST)
            set(Python_FIND_STRATEGY LOCATION)
            set(Python_FIND_FRAMEWORK LAST)
            find_package(Python 3.10.0...<3.13.0 REQUIRED COMPONENTS Interpreter Development)
            Python_add_library(
                ${LF_MAIN_TARGET}
                MODULE

@MoezBHH
Copy link
Collaborator

MoezBHH commented Dec 6, 2024

A missing Py_DECREF() caused a segmentation fault in Python's C extensions. After the PyObject_CallObject(callable, arglist) call, we need to decrease the reference count of the arglist object.

Now, the required Python version is 3.10.0 and newer versions.

@edwardalee edwardalee requested a review from MoezBHH December 8, 2024 01:18
Copy link
Collaborator

@MoezBHH MoezBHH left a comment

Choose a reason for hiding this comment

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

All regression tests passed on my laptop with Ubuntu 24.10 and Python 3.12.7.

@edwardalee
Copy link
Collaborator Author

edwardalee commented Dec 9, 2024

All the tests also passed on my laptop with Python 3.13.0. Any ideas why CI is stuck? In this commit: 33cc862 I've changed the Python version to fix it at 3.12. I suspect that the code that was there was for testing with 3.10, 3.11, and 3.12, but that was probably what was blocking.

@edwardalee edwardalee enabled auto-merge December 9, 2024 15:47
@edwardalee edwardalee added this pull request to the merge queue Dec 9, 2024
Merged via the queue into master with commit 7adcb51 Dec 9, 2024
23 checks passed
@edwardalee edwardalee deleted the python-12 branch December 9, 2024 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants