-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Added support for loading Python at runtime (supports both Python 3 and 2) #20674
Conversation
} | ||
|
||
for majorVersion in supportedMajorVersions { | ||
for minorVersion in supportedMinorVersions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be more elegant if there were simply an array of tuples called supportedVersions: [(major: Int, minor: Int?)]
?
Such a tuple could also be manually populated (i.e., [(3, nil), (3, 8), (3, 7), /* ... */]
). There's no need for supporting nonexistent Python versions (indeed, that would be an oxymoron).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing support for "nonexistent" Python versions means that old Swift distributions (or even any distributed executable that statically linked the Python module) would not work with any 3.x Python version released after it, even though the Python ABI is marked as stable for all the Python 3 lifetime, so it should work. Also, it would require maintaining the list and deciding when it's time to add new versions.
Take in account that while in Windows the Python distribution includes both python3.dll
and python37.dll
in macOS and Linux we have to specify the exact minor version to load it (Python.framework/Versions/3.7/Python
or libpython3.7m.so
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the ABI is stable for all versions of Python 3, then is there a need for a list of supported minor versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xwu Read the second paragraph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid I don't understand. Why do we need a list of supported minor versions? If a specific version isn't specified, you'd want to load the most recent version of Python 3 regardless of what minor version it is, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xwu When we dynamically load Python with dlopen
we need to know the filename of the library, but as each minor version has a different version we have to try each one until one loads. For example in macOS:
$ PYTHON_LOADER_LOGGING=1 PYTHON_VERSION=3 PythonTool
Loading symbol 'Py_Initialize' from the Python library...
Trying to load library at 'Python.framework/Versions/3/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3/Python'...
Trying to load library at 'Python.framework/Versions/3.30/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.30/Python'...
Trying to load library at 'Python.framework/Versions/3.29/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.29/Python'...
Trying to load library at 'Python.framework/Versions/3.28/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.28/Python'...
Trying to load library at 'Python.framework/Versions/3.27/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.27/Python'...
Trying to load library at 'Python.framework/Versions/3.26/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.26/Python'...
Trying to load library at 'Python.framework/Versions/3.25/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.25/Python'...
Trying to load library at 'Python.framework/Versions/3.24/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.24/Python'...
Trying to load library at 'Python.framework/Versions/3.23/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.23/Python'...
Trying to load library at 'Python.framework/Versions/3.22/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.22/Python'...
Trying to load library at 'Python.framework/Versions/3.21/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.21/Python'...
Trying to load library at 'Python.framework/Versions/3.20/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.20/Python'...
Trying to load library at 'Python.framework/Versions/3.19/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.19/Python'...
Trying to load library at 'Python.framework/Versions/3.18/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.18/Python'...
Trying to load library at 'Python.framework/Versions/3.17/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.17/Python'...
Trying to load library at 'Python.framework/Versions/3.16/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.16/Python'...
Trying to load library at 'Python.framework/Versions/3.15/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.15/Python'...
Trying to load library at 'Python.framework/Versions/3.14/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.14/Python'...
Trying to load library at 'Python.framework/Versions/3.13/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.13/Python'...
Trying to load library at 'Python.framework/Versions/3.12/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.12/Python'...
Trying to load library at 'Python.framework/Versions/3.11/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.11/Python'...
Trying to load library at 'Python.framework/Versions/3.10/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.10/Python'...
Trying to load library at 'Python.framework/Versions/3.9/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.9/Python'...
Trying to load library at 'Python.framework/Versions/3.8/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.8/Python'...
Trying to load library at 'Python.framework/Versions/3.7/Python'...
Trying to load library at '/usr/local/Frameworks/Python.framework/Versions/3.7/Python'...
Library at '/usr/local/Frameworks/Python.framework/Versions/3.7/Python' was sucessfully loaded.
Loading symbol 'PyEval_GetBuiltins' from the Python library...
Loading symbol 'Py_IncRef' from the Python library...
Loading symbol 'PyImport_ImportModule' from the Python library...
Loading symbol 'PyObject_GetAttrString' from the Python library...
Loading symbol 'Py_DecRef' from the Python library...
Loading symbol 'PyLong_AsLong' from the Python library...
Loading symbol 'PyErr_Occurred' from the Python library...
[*] Python 3.7
[ ] Version: 3.7.1
And on Linux:
$ PYTHON_LOADER_LOGGING=1 PYTHON_VERSION=3 PythonTool
Loading symbol 'Py_Initialize' from the Python library...
Trying to load library at 'libpython3.so'...
Trying to load library at 'libpython3m.so'...
Trying to load library at 'libpython3.30.so'...
Trying to load library at 'libpython3.30m.so'...
Trying to load library at 'libpython3.29.so'...
Trying to load library at 'libpython3.29m.so'...
Trying to load library at 'libpython3.28.so'...
Trying to load library at 'libpython3.28m.so'...
Trying to load library at 'libpython3.27.so'...
Trying to load library at 'libpython3.27m.so'...
Trying to load library at 'libpython3.26.so'...
Trying to load library at 'libpython3.26m.so'...
Trying to load library at 'libpython3.25.so'...
Trying to load library at 'libpython3.25m.so'...
Trying to load library at 'libpython3.24.so'...
Trying to load library at 'libpython3.24m.so'...
Trying to load library at 'libpython3.23.so'...
Trying to load library at 'libpython3.23m.so'...
Trying to load library at 'libpython3.22.so'...
Trying to load library at 'libpython3.22m.so'...
Trying to load library at 'libpython3.21.so'...
Trying to load library at 'libpython3.21m.so'...
Trying to load library at 'libpython3.20.so'...
Trying to load library at 'libpython3.20m.so'...
Trying to load library at 'libpython3.19.so'...
Trying to load library at 'libpython3.19m.so'...
Trying to load library at 'libpython3.18.so'...
Trying to load library at 'libpython3.18m.so'...
Trying to load library at 'libpython3.17.so'...
Trying to load library at 'libpython3.17m.so'...
Trying to load library at 'libpython3.16.so'...
Trying to load library at 'libpython3.16m.so'...
Trying to load library at 'libpython3.15.so'...
Trying to load library at 'libpython3.15m.so'...
Trying to load library at 'libpython3.14.so'...
Trying to load library at 'libpython3.14m.so'...
Trying to load library at 'libpython3.13.so'...
Trying to load library at 'libpython3.13m.so'...
Trying to load library at 'libpython3.12.so'...
Trying to load library at 'libpython3.12m.so'...
Trying to load library at 'libpython3.11.so'...
Trying to load library at 'libpython3.11m.so'...
Trying to load library at 'libpython3.10.so'...
Trying to load library at 'libpython3.10m.so'...
Trying to load library at 'libpython3.9.so'...
Trying to load library at 'libpython3.9m.so'...
Trying to load library at 'libpython3.8.so'...
Trying to load library at 'libpython3.8m.so'...
Trying to load library at 'libpython3.7.so'...
Trying to load library at 'libpython3.7m.so'...
Trying to load library at 'libpython3.6.so'...
Trying to load library at 'libpython3.6m.so'...
Trying to load library at 'libpython3.5.so'...
Trying to load library at 'libpython3.5m.so'...
Library at 'libpython3.5m.so' was sucessfully loaded.
Loading symbol 'PyEval_GetBuiltins' from the Python library...
Loading symbol 'Py_IncRef' from the Python library...
Loading symbol 'PyImport_ImportModule' from the Python library...
Loading symbol 'PyObject_GetAttrString' from the Python library...
Loading symbol 'Py_DecRef' from the Python library...
Loading symbol 'PyLong_AsLong' from the Python library...
Loading symbol 'PyErr_Occurred' from the Python library...
[*] Python 3.5
[ ] Version: 3.5.2
As you can see, neither in macOS nor on Linux, the major version is enough to locate a valid library (Python.framework/Versions/3/Python
/libpython3.so
) so we have to try all possible minor versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that. What I don't understand is why we need a static array of "supported" minor versions if support is not contingent on the minor version. How many minor versions you try is arbitrary and an implementation detail of the code that loads the library, having nothing to do with what versions are supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what would you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static func loadPythonLibrary() -> UnsafeMutableRawPointer? {
if let pythonLibraryPath = Environment.library.value {
return loadPythonLibrary(at: pythonLibraryPath)
}
let triedMinorVersions: [Int?] = [nil] + Array(0...30).reversed()
for majorVersion in supportedMajorVersions {
for minorVersion in triedMinorVersions {
for libraryPath in libraryPaths {
let version = PythonVersion(major: majorVersion, minor: minorVersion)
guard let pythonLibraryHandle = loadPythonLibrary(
at: libraryPath, version: version) else {
continue
}
return pythonLibraryHandle
}
}
}
return nil
}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can work on this in a follow-up.
There will never be a version of Python 2 after 2.7, for instance, and it makes no sense to look for one. If "Python 2.30" existed, we certainly wouldn't want to support it, since what would that even be? Likewise, is Python 2.0 really supported? (In a similar vein, I'm not sure that Python 3.30, if it ever exists, should be supported today; at the current clip of one minor release every year or so, that'd be 20 years in the future, and if that were to happen who knows what that version would look like?) This is all to say that iterating over the same minor versions for every major version is not ideal.
} | ||
|
||
// Methods of `PythonLibrary` required to load the Python library. | ||
extension PythonLibrary { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the extension private and drop ‘private’ from members.
return pythonLibraryHandle | ||
} | ||
|
||
private static func getPythonLibraryHandle() -> UnsafeMutableRawPointer? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using “get” or “set” in function names.
Plus, I think this can be a computed property.
} | ||
|
||
// Methods of `PythonLibrary` used for logging messages. | ||
extension PythonLibrary { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used in other files? If not, make the extension private.
} | ||
|
||
// Methods of `PythonLibrary` required to read the environment variables. | ||
extension PythonLibrary { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be private too?
|
||
// Methods of `PythonLibrary` used for logging messages. | ||
extension PythonLibrary { | ||
static func log(message: String) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argument label “message” doesn’t add any clarity. Why not just log(_:)?
} | ||
|
||
static func loadSymbol<T>( | ||
name: String, legacyName: String? = nil, signature: T.Type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
“signature” is an overloaded word. I’d suggest using “type:”.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
We will also need an API that enables the user to select a Python version. Something like: Python.useVersion(3, 6)
. It can be added in a separate patch.
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove empty line.
|
||
// Methods of `PythonLibrary` required to load the Python library. | ||
private extension PythonLibrary { | ||
static let supportedMajorVersions: [Int] = Array(2...3).reversed() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please just write [3, 2]
.
The function should be on Something like this would do the trick: // Methods of `PythonLibrary` required to set the Python version.
extension PythonLibrary {
private static func useVersion(_ version: PythonVersion) {
PythonLibrary.Environment.version.set(version.versionString)
}
public static func useVersion(_ major: Int, _ minor: Int? = nil) {
let version = PythonVersion(major: major, minor: minor)
self.useVersion(version)
}
} Example: import Python
PythonLibrary.useVersion(2) // Should be called before any references to `Python`
let sys = try Python.attemptImport("sys")
print(sys.version) |
IMO, using a public extension PythonLibrary {
static func useVersion(_ version: Float) {
... // do version tuple conversion here and set environment
}
} |
Take in account that the The public method would be: PythonLibrary.useVersion(2)
// or
PythonLibrary.useVersion(2, 7) |
Ok, I'm sold.
Adding a single method for now and making |
Great, done! 👍🏻 |
@swift-ci please test tensorflow |
CI is dead. Gotta wait a while. |
@swift-ci please test tensorflow |
Could you add new files to |
@rxwei Should I also remove the |
@pvieito Yes, please remove |
@swift-ci Please test tensorflow |
@dan-zheng could you restart the test, please? I have fixed this error:
But the ones referring to |
I believe the Could you try building PythonKit on Linux with those extra flags? Unfortunately, this seems like a libSyntax deficiency. EDIT: It seems that libSyntax does not yet support // This passes syntax verification.
let f: @convention(c) (Int) -> Int = { x in x }
func foo<T>(_ x: T.Type) {}
// This does not.
foo((@convention(c) (Int) -> Int).self) $ swiftc -Xfrontend -verify-syntax-tree convention.swift
convention.swift:6:6: error: unknown expression syntax exists in the source
foo((@convention(c) (Int) -> Int).self)
^ I filed SR-9316 to track this. |
@dan-zheng Exactly:
|
@pvieito Hmm, that's unfortunate. Unless we can disable @nkcsgexi: Could you please provide some advice on how to add libSyntax support for this usage of |
The libSyntax issue is a bit surprising. There's no instance of getting the metatype of a static func loadSymbol<T>(
name: String, legacyName: String? = nil, type: T.Type = T.self
) -> T Then change your call sites to provide a contextual type. let PyList_New: @convention(c) (Int) -> PyObjectPointer? =
PythonLibrary.loadSymbol(name: "PyList_New") |
@swift-ci please test tensorflow |
1 similar comment
@swift-ci please test tensorflow |
@swift-ci please clean test tensorflow |
It's still failing possibly because of incorrectly configured cmake. You mentioned you couldn't test it on your machine, is that right? If so, I can take it from here. |
Yes, exactly, and I am not very familiar with cmake.
Great, thanks! |
It seems that we have to reference the symbol: guard isType(pythonObject, type: PySlice_Type) else { return nil } Not its address: guard isType(pythonObject, type: &PySlice_Type) else { return nil } // Crashes |
Indeed! We didn't encounter a test case that would make |
Nvm. Pushed. |
…ent to `isType(_:type:)`.
@swift-ci please test tensorflow |
@rxwei Sure! I suppose we should modify the symbol definition (to match the real Python C header) instead of the calling site? |
Symbol definitions are correct. It's just that we don't need these type variables to be |
Ok, great! |
- `()` -> `Void`. - Drop parentheses at the result position from single-result function signatures. - Replace !(==) with != in `isType(_:type:)`.
@rxwei Please, remove also the symbol |
@swift-ci please test tensorflow macOS |
4 similar comments
@swift-ci please test tensorflow macOS |
@swift-ci please test tensorflow macOS |
@swift-ci please test tensorflow macOS |
@swift-ci please test tensorflow macOS |
@pvieito 我想知道在Mac上怎么使用指定版本的python,现在我使用PythonLibrary.useVersion(3),会崩溃Python library not found. Set the PYTHON_LIBRARY environment variable with the path to a Python library.但是我又找不到方法去指定PYTHON_LIBRARY |
Added new PythonLibrary struct to load Python library and symbols dynamically at runtime