-
Notifications
You must be signed in to change notification settings - Fork 18
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 and Skia don't mix #32
Comments
Hello, The problem is specifically in the MaskFPUExceptions function in PythonEngine.pas, which is used in the initialization of P4D-Data- Sciences There are a few ways to resolve this. One of them would be to create a class to store the mask's state, change it, so that after a try finally you can restore the previous mask. |
@viniciusfbb Thanks for the rapid reply and explanation. I sorta thought this was most likely MaskFPUExceptions (hence the issue being tentatively in this repo) I wrote a replacement for MaskFPUExceptions that stores the current GetExceptionMask when called with True in a global then replaces it when called with false. This worked a treat and Skia now works perfectly again (Skia was really acting up without this fix...). @lmbelo While my hack works I'm not submitting a PR for this one as this is a complicated issue which is situational - my 'fix' works for my app but other packages may well mess with the Exception Mask so a more robust solution is really required. For reference here's what I did - you can enable/disable it via the define (good for testing)...
|
The exception mask is undone after the importation of any of the Python packages. If you have a look at the exception mask routine you'll see that the exception mask is being undone (for CPUX86 and CPUX64) to Delphi's default. |
Ahh, that explains why using afterimport to reset the mask didn't work... I tried two methods... import(packagea) with each of those using shared beforeinstall, afterinstall, beforeimport, afterimport which calls my SafeMaskFPUExceptions() function - this didn't work :( So, I've not got SafeMaskFPUExceptions(True) See https://github.com/peardox/Lartis/blob/main/PythonSystem.pas#L501 |
You should decorate it by a try finally block.
|
I'm going to - this was a recent test, still unfinished - no proper error handlers yet I just discovered that I need to handle errors properly anyway (Linux screws up horribly ATM - Issue coming soon...) |
You need to get extra attention when targeting installed Python versions on Linux. You might hit across known issues reported before here. Have a look in all closed issues when you're going afters Linux errors. |
I was using an Embedded Python 3.9 - which is why the issue is listed here |
Our embeddables should work fine on any of the provided platforms. |
@lmbelo, you didn't understand my post. The P4D-Data-Sciences code is wrong and python4delphi's MaskFPUExceptions implementation should be changed as well. I will explain it better. MaskFPUExceptions code from python4delphi: python4delphi/Source/PythonEngine.pas Lines 9288 to 9306 in 19192ad
Note that the code above arbitrarily changes the exception mask to:
Now, look at one of the P4D-Data-Sciences import code: procedure TTorchVision.ImportModule;
begin
MaskFPUExceptions(true);
try
inherited;
finally
MaskFPUExceptions(false);
end;
end; As it stands, if the exception mask before the code above is empty [], after import it will be So, the correct thing is: save the mask value, change it and after importing the P4D-Data-Sciences, restore the previous mask value and not arbitrarily change it to I made a possible fix for both codes. First for python4delphi's MaskFPUExceptions: procedure MaskFPUExceptions(ExceptionsMasked: Boolean;
var StoredMask: TArithmeticExceptionMask; MatchPythonPrecision: Boolean);
begin
{$IF Defined(CPUX86) or Defined(CPUX64)}
if ExceptionsMasked then
StoredMask := SetExceptionMask(GetExceptionMask + [exInvalidOp, exDenormalized, exZeroDivide,
exOverflow, exUnderflow, exPrecision])
else
SetExceptionMask(StoredMask + [exDenormalized, exUnderflow, exPrecision]); // Maybe a SetExceptionMask(StoredMask); would be better, but I'm not sure if removing the " + [exDenormalized, exUnderflow, exPrecision]" would cause problems in python4delphi
{$WARN SYMBOL_PLATFORM OFF}
{$IF Defined(FPC) or Defined(MSWINDOWS)}
if MatchPythonPrecision then
SetPrecisionMode(pmDouble)
else
SetPrecisionMode(pmExtended);
{$WARN SYMBOL_PLATFORM ON}
{$IFEND}
{$IFEND}
end; And now one of the P4D-Data-Sciences import codes: procedure TTorchVision.ImportModule;
var
StoredMask: TArithmeticExceptionMask;
begin
MaskFPUExceptions(true, StoredMask);
try
inherited;
finally
MaskFPUExceptions(false, StoredMask);
end;
end; |
@viniciusfbb does Skia changes the exception mask for the whole app life cycle? |
Just to complicate things ARM can emulate the FPU mask and Mac can be x64 or aarch64 |
Yes! OpenGL too. |
As required by TWebBrowser, but it only changes as needed, right? Many external libraries changes it and restores as done. |
We could stand with the following:
Otherwise, I'd need to change it in P4D. |
We are only changing the exception mask per invocation, otherwise we would get it incompatible with many other libraries. |
Changing the exception mask in general is already a risk, even if temporarily, as it affects the entire application, even threads running in parallel. But the biggest risk is in removing exceptions from the mask. That's why I suggested changing the code of python4delphi.PythonEngine.MaskFPUExceptions() to give include in the exception mask instead of completely replacing it to a new value. |
Well, regarding Windows x86 (made by FLDCW and FSTCW ???) and x64, the FPSCR flag equivalent is saved across thread context switches. I'm almost sure ARM has a similar approach, I think they save it in the _FPSCR array (I need to confirm this). |
Not quite. Change the main, run a thread and you will see the value of the mask of the main in the thread... Until a few years ago SetExceptionMask was not even thread safe, but I believe this has already been fixed. Anyway, just changing the import of P4D-Data-Sciences the way you suggest is not the way I think is the most correct but it will hardly cause problems. |
I didn't say that changes to background threads would affect the main thread's exception mask. In my view, threads being created in the background would have the initial values of exception mask of the main thread. However, I have confirmed it through testing, and it is not true. New threads copy the values of mask exceptions from the thread that created it, not from the main thread. So your change is safe too. |
@viniciusfbb let me see where Skia is changing exception masks. By now, let's stand to the adjustments in the DSC components only. |
An important concern is what happens when you call python outside P4D e.g. I'm using my hack above like this
I'm guessing I should also wrap e.g. As well. MainModule.delphi_train(); can take a few hours to run (and will be using Skia once a second for reporting) - I'll have to give that a go this week |
That's why exception masks should be changed and restored per invocation. |
Btw, how are you running a blocking task on main thread and using Skia to report it out of a second thread? |
What's exacrly what SafeMaskFPUExceptions() does. |
There are numerous operations that depend on masks in exceptions, which are executed every frame of the app's rendering, so we decided to change it already on load. |
Just starting a new thread for the module call then hand back to Delphi Yesterday they finally told us they have their own task manager (but it'll also be setting the mask to bad values I guess) |
I understand. My concern is that it would affect other libraries out of our sight. |
You must be careful while using the Python packages out of the main thread. |
This seems to work...
It uses a Skia shader while the thread is running showing data reported back from that thread (it's a boring progress display) |
I'm not positive about the exact criteria yet
Basically imports with Skia fully enabled causes Skia to hit problem with floating point
Take anything with an import and add this to your dproj (after installing Skia of course) and everything will go badly wrong.
Masking the FPU exception doesn't seem to help
As I don't know which package is causing the issue this is posted here
The text was updated successfully, but these errors were encountered: