-
Notifications
You must be signed in to change notification settings - Fork 316
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.10 #769
Python 3.10 #769
Conversation
As title
As title
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@sklam and I agreed that a 0.37.1 may be in the cards for next week. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
llvmlite/binding/ffi.py
Outdated
@@ -183,7 +183,8 @@ def __call__(self, *args, **kwargs): | |||
for _lib_path in _lib_paths: | |||
try: | |||
lib = ctypes.CDLL(_lib_path) | |||
except OSError: | |||
except OSError as e: | |||
print(e) |
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 leftover from debugging?
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.
yes probably, but we may to leave it it. This solves the issue, where errors of the form libllvmlite.so could not be loaded
masquerade the real error, which makes these kinds of errors hard to debug. If the lib
can not be loaded, we can't continue anyway, so maybe a lone print
here is the right thing to do?
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'd prefer buffering the messages and report it later when it does raise OSError
at
llvmlite/llvmlite/binding/ffi.py
Line 192 in 334c000
raise OSError("Could not load shared object file: {}".format(_lib_name)) |
Right now, if the last path is successful, the earlier error are still being printed.
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.
ok, that makes sense
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.
Fixed in: 8cc3c39
Here is a sample failure:
Python 3.9.5 (default, May 18 2021, 12:31:01)
[Clang 10.0.0 ] :: Anaconda, Inc. on darwin
Type "help", "copyright", "credits" or "license" for more information.
i>>> import llvmlite
>>> import llvmlite.binding
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/vhaenel/git/llvmlite/llvmlite/binding/__init__.py", line 4, in <module>
from .dylib import *
File "/Users/vhaenel/git/llvmlite/llvmlite/binding/dylib.py", line 3, in <module>
from llvmlite.binding import ffi
File "/Users/vhaenel/git/llvmlite/llvmlite/binding/ffi.py", line 195, in <module>
raise OSError(msg)
OSError: Could not load shared object file: libllvmlite.dylib
Errors were: [OSError('dlopen(/Users/vhaenel/git/llvmlite/llvmlite/binding/libllvmlite.dylib, 6): image not found'), OSError('dlopen(libllvmlite.dylib, 6): image not found'), OSError('dlopen(./libllvmlite.dylib, 6): image not found'), OSError('dlopen(/Users/vhaenel/git/llvmlite/llvmlite/binding/libllvmlite.dylib, 6): image not found')]
This will avoid printing errors unnecessarily.
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've manually tested the failed to find libllvmlite error and it looks good.
I've manually tested this on OSX on py3.10. ✅ |
The current CI failures:
Are because the packages related to documentation are not available yet via the |
So, what should we do. I see three options: a) Don't try to build the docs for 3.10 We also still need to make changes to the anaconda internal build farm in order to build the conad-packages for 3.10 and the wheels too. |
Build Farm ID: |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Pipelines successfully started running 1 pipeline(s). |
#789 is now provided to circumvent the CI issues present on this PR. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Oh wow, it went green for the first time, yay! |
Now running on the Anconda internal build farm as: |
Matti tracked the issue Marcel noted above to the finalization line here (622): llvmlite/llvmlite/tests/test_binding.py Lines 621 to 625 in d77dc1b
More details in comment ( conda-forge/llvmlite-feedstock#58 (comment) ) |
@jakirkham thank you for posting this. It will probably make sense to debug this during the RC period when we have also built our binary artifacts. |
See conda-forge/llvmlite-feedstock#58 (comment) : Building works fine in conda-forge's setup but testing with |
@esc , this could/would also mean that you can probably test it in an environment created with your build artifact and |
@esc, as an update: the segfault on osx-64 for conda-forge stems from the CPython 3.10 plus libffi 3.4 combination. TL;DR looks like |
Great finds! Thank you for keeping us in the loop. It is much appreciated! |
@mbargull would you recommend we pin |
No the issue was in how CPython was doing the build itself. We fixed that in conda-forge ( conda-forge/python-feedstock#525 ). Apparently the Python devs also fixed this, but it didn't make it into 3.10.0 ( https://bugs.python.org/issue45350 ). |
Yep, what John wrote! All good from our side now. I'll open an issue on Anaconda's recipe repo (\edit: AnacondaRecipes/python-feedstock#50 ) too so we don't run into the same issue for the |
@sklam @stuartarchibald @seibert I think this one is ready, needs a final review, I believe. |
@mbargull Thank you, much appreciated!! 🙏 |
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.
It looks like all the issues brought up in the discussion are now resolved, so this looks good to me.
Thank you all for the reviews! |
As title