[MichaelPlug]

I added a new function to check if the PythonLibrary is valid or not, to avoid the fatal error during the importing of sys and to try a different path to import PythonLibrary.
I think it is a useful function because from MacOs 12.3 we can't assume that all users have Python 2 on their MacBook at the same path.

[philipturner]

Could you add a test case for that to PythonRuntimeTests.swift? It doesn't mean you have to run the tests in an environment with/without Python 2. You just need to assert that the function compiles and executes without crashing on all platforms.

Furthermore, call that function again at the end of an existing test. The dedicated test case might end up always being the first to execute. Thus, it would never validate whether the function crashes after the runtime has been fired up.

One more suggestion - what about refactoring the function into a computed get-only property called libraryExists? The verb "check" sounds a bit generic.

[MichaelPlug]

In which test function should I add this function? Sorry, it's not clear to me. It's look that functions useLibrary and useVersion are never called. @philipturner
Thank you for your suggestions.

[philipturner]

Why is libraryExists a function instead of a computed property? In my opinion, it seems semantically like something that should be a property. It takes no arguments, has a return value, and is not an action verb in English language.

In which test function should I add this function?

Just XCTAssertEqual between libraryExists and true at the beginning of testCheckVersion(). If it would have returned false, then that function would have caused a runtime crash anyway.

It's look that functions useLibrary and useVersion are never called.

Those functions are dependent on the computer's configuration and don't always work. For example, PythonKit crashed when I tried calling useVersion on the old Swift for TensorFlow tutorials with my new Swift-Colab. I had to use useLibrary before on my Mac to run a PyTorch script from Swift (gist here). But libraryExists should always return true in the Swift package tests.

[philipturner]

I just initiated a review, but I'm not quite sure whether you actually see it because it says pending. Could you validate that you see a code suggestion?

[pvieito]

Hi! Please see this commit: 6e2c366

There is now a new PythonLibrary.loadLibrary() throwing function that you can use to check if a Python library is available by catching the error.

[philipturner]

The reason I asked to have someone validate my attempt at starting a review (#55 (comment)) has nothing to do with this bug in particular. Could someone on this thread tell me whether they see the review?

Edit: I would like both other people to validate, because GitHub's settings might be different for the repository owner.

[pvieito]

test

[pvieito]

test2

[pvieito]

@philipturner To "publish" your review you have to click in the "Finish Review" button.

[philipturner]

Suggested change

	public static func libraryExists() -> Bool {
	let pythonLibraryHandleTest = Self.loadPythonLibrary()
	return Self.isPythonLibraryLoaded(at: pythonLibraryHandleTest)
	}
	public static var libraryExists: Bool {
	let pythonLibraryHandleTest = Self.loadPythonLibrary()
	return Self.isPythonLibraryLoaded(at: pythonLibraryHandleTest)
	}

[philipturner]

I had no idea that was how you did a review!!! I wish I knew that months ago! Thanks.

[MichaelPlug]

Great solution, thank you very much for your work

[MichaelPlug]

I'm trying to use the new function loadLibrary(), but seems that it is not enough to resolve my problem loading Python on some device and on MacOS 13.3.1.
I'll give you an example of a use case:

pythonIsWorking = false

PythonLibrary.useLibrary(WRONGPATH)
do {
      try PythonLibrary.loadLibrary()
      pythonIsWorking = true
}
catch{
     PythonLibrary.useLibrary(RIGHTPATH)
     do {
            try PythonLibrary.loadLibrary()
            pythonIsWorking = true
      }
      catch{
            pythonIsWorking = false      
      }

With this code I would expect that the final value of pythonIsWorking should be true, but it seems that the line PythonLibrary.useLibrary(RIGHTPATH) is useless and is not possible to reset the Python path after initialized it.
It leads that loadLibrary function si not very usefull. I already tested that with switching WRONGPATH and RIGHTPATH pythonIsWorking is setted to true.

[pvieito]

It is working for me:

import PythonKit

var pythonIsWorking = false
PythonLibrary.useLibrary(at: "/invalid/path")
do {
    try PythonLibrary.loadLibrary()
    pythonIsWorking = true
}
catch {
    print("Error 1:", error)
    PythonLibrary.useLibrary(at: "/opt/homebrew/opt/python@3.9/Frameworks/Python.framework/Python")
    do {
        try PythonLibrary.loadLibrary()
        pythonIsWorking = true
    }
    catch {
        print("Error 2:", error)
        pythonIsWorking = false
    }
}
    
print("Python is Working: \(pythonIsWorking)")

$ PythonTool
Error 1: Python library not found. Set the PYTHON_LIBRARY environment variable with the path to a Python library.
Python is Working: true

[MichaelPlug]

I was too hasty, I'm trying to explain the problem better.
If I try to override the path it works, but if I use pythonLibrary.useVersion() it's seems that it not.

The correct example is:

PythonLibrary.useLibrary(WRONGPATH)
do {
      try PythonLibrary.loadLibrary()
      pythonIsWorking = true
}
catch{
     PythonLibrary.useVersion(3)
     do {
            try PythonLibrary.loadLibrary()
            pythonIsWorking = true
      }
      catch{
            pythonIsWorking = false      
      }

But if I do this it works:

PythonLibrary.useVersion(3)
do {
      try PythonLibrary.loadLibrary()
      pythonIsWorking = true
}
catch{
      PythonLibrary.useLibrary(WRONGPATH)
     do {
            try PythonLibrary.loadLibrary()
            pythonIsWorking = true
      }
      catch{
            pythonIsWorking = false      
      }

It's seems that for my issue a simple solution is try before the function useLibary() and then useVersion(), but probably this stuff can be improved.

Thank you for your help and I'm sorry if I didn't explained the problem clearly,

[pvieito]

Gotcha! Since the PYTHON_LIBRARY environment variable takes preference over the PYTHON_VERSION variable, I have just added support for clearing both variables in this commit:

PythonLibrary.useVersion(nil)
PythonLibrary.useLibrary(at: nil)