lazy import of multiprocessing

[vasole]

Alternative solution to #1185

[vasole]

It is either this solution of the already merged one.

[vasole]

I do not care which one you take.

Please feel free to close without merging.

[woutdenolf]

There are 4 files that configure the generation of a frozen application:

https://github.com/search?q=repo%3Asilx-kit%2Fpymca+excludes.append%28%22multiprocessing%22%29&type=code

Three of these exclude multiprocessing and one includes multiprocessing in the frozen application.

[woutdenolf]

#1185 got merged by the reviewer.

FYI our current policy is that the reviewer approved or rejects. And the author merges when approved.

[sergey-yaroslavtsev]

Distributed packages in both cases includes MP because Windows and MacOS has commented excludes.append("multiprocessing").
If it would be added the tests will fail because one them call run_in_subprocess explicitly.

1. Thus for me "lazy" import should be done like this The workaround for HDF5Utils #1188 (where the test and builds were corrected) which was closed.
2. Or MP should be excluded from the binaries + test should be fixed and we keep what was merged via easy fix for MP in frozen MacOS binaries #1185.

Anyway we need to change freezing and test - that is why i am confused with both approaches of @vasole.

3. Or we keep what was initially was done in easy fix for MP in frozen MacOS binaries #1185 which includes MP directly to PyMca. Then freezing and test stay as is.

[woutdenolf]

The current situation in master:

• 3 frozen application configuration include multiprocessing.
• 1 frozen application configuration excluded multiprocessing.
• we have try ... except ... around all multiprocessing imports and select an alternative in safe_hdf5_group_keys.
• freeze_support is called in all CLI applications when multiprocessing is available.

So in this state

• testHDF5Utils.testSegFault is missing a pytest.skip when multiprocessing is not available. So add another try ... except ... for this in the tests.
• not "all" CLI applications call freeze_support. There are 22 CLI applications.

[woutdenolf]

The situation in this branch:

• Failing run_in_subprocess due to missing multiprocessing in safe_hdf5_group_keys and select an alternative in that case.

So in this state

• testHDF5Utils.testSegFault is missing a pytest.skip when multiprocessing is not available. So add another try ... except ... for this in the tests.

[vasole]

The current situation in master:

• 3 frozen application configuration include multiprocessing.
• 1 frozen application configuration excluded multiprocessing.
• we have try ... except ... around all multiprocessing imports and select an alternative in safe_hdf5_group_keys.
• freeze_support is called in all CLI applications when multiprocessing is available.

I would like to add that all those frozen configurations work.

[woutdenolf]

I would like to add that all those frozen configuration work.

So to be clear, it is an explicit choice to have 3 configurations that include multiprocessing and 1 configuration that doesn't.

[woutdenolf]

So by comparing the state of the master and this MR in addition with things that need fixing in both cases I would vote for this MR with the addition of using pytest.skip in testHDF5Utils.testSegFault in case multiprocessing is not available.

[vasole]

FYI our current policy is that the reviewer approved or rejects. And the author merges when approved.

Ok.

It is unusual because the generic case is that one does not always have the rights to merge. If you have decided that I will of course adhere.

The other generic case that I do not see applied in this repository is that the development takes place in a forked repository. For minor changes I can understand the "patch branches" made through the web interface.

[vasole]

• testHDF5Utils.testSegFault is missing a pytest.skip when multiprocessing is not available. So add another try ... except ... for this in the tests.

Done. It was contemplated in #1188

[woutdenolf]

development takes place in a forked repository

Also this changed. We mostly use the main repo these days. Some silx examples:

• Integrate what can be from https://github.com/silx-kit/silx/pull/4361 silx#4445
• silx.gui.plot.actions.histogram: Fixed PixelIntensitiesHistoAction to include max value in histogram silx#4469

[vasole]

So to be clear, it is an explicit choice to have 3 configurations that include multiprocessing and 1 configuration that doesn't.

Yes. @sergey-yaroslavtsev made sure that old and new configuration files were available.

[woutdenolf]

After fixing the typo I think this MR is good to go. I'll let @sergey-yaroslavtsev approve or possible comment on things I missed.

[vasole]

Also this changed. We mostly use the main repo these days

Ok although I consider it a mistake for two reasons:

• When the main repository has to pay for CI platforms (AppVeyor, TravisCI) it is a waste of money and time for the main repository. I guess it will change again if we run into money and time issues.

• it is instructive for developers to get acquainted with normal development practices when contributing to open source projects not owned by them.

[woutdenolf]

So to be clear, it is an explicit choice to have 3 configurations that include multiprocessing and 1 configuration that doesn't.

Yes. @sergey-yaroslavtsev made sure that old and new configuration files were available.

As I understand we have this in terms of multiprocessing inclusion

Platform	Tool	Output	Type	multiprocessing (github)	multiprocessing (original)
Windows	PyInstaller	PyMcaMain.exe	Standalone executable	yes	no
Windows	cx_Freeze	pymca*.exe	Installer (EXE)	yes	yes
macOS	PyInstaller	*.app and *.dmg (arm64/x86_64/Universal2)	App bundle + DMG	yes	no

grep -r 'excludes.append("multiprocessing")' .

./package/pyinstaller/pyinstaller.spec:excludes.append("multiprocessing")
./package/pyinstaller/pyinstaller_github.spec:# excludes.append("multiprocessing")
./package/cxfreeze/cx_setup_github.py:#excludes.append("multiprocessing")
./package/cxfreeze/cx_setup.py:#excludes.append("multiprocessing")

Edit: I messed up yes/no and # or not. Should be ok now in the table.

[woutdenolf]

When the main repository has to pay for CI platforms (AppVeyor, TravisCI) it is a waste of money

The CI runs only on merge requests and push to master. Not sure how we get less jobs by creating PR's from personal forks.

[sergey-yaroslavtsev]

As I understand we have this in terms of multiprocessing inclusion

Indeed. Just a curious thing - cx_freeze had MP before. Just Windows version does not require freeze_support for every single use of MP.

Should be ok now in the table.

yes the last version looks correct.

[vasole]

As I understand we have this in terms of multiprocessing inclusion

In the original cx_freeze I would have expected multiprocessing to be excluded as it was the case for PyInstaller but I have checked in v5.9.4 and it was like it is now.

It worked because cx_Freeze was only used under windows and that HDF5Utils.py was never called under frozen binary. When you added the HDF5 sorting method to it (before it was inside another module) it would have been broken but the changes of @sergey-yaroslavtsev made it work.

When this PR gets merged, all freezing approaches will work again because the lazy import solves the issue created by moving the sorting method to HDF5Utils.py

[vasole]

The CI runs only on merge requests and push to master. Not sure how we get less jobs by creating PR's from personal forks.

Because more immature merge requests are made since testing is not done in a "separate" personal fork.

[sergey-yaroslavtsev]

Because more immature merge requests are made since testing is not done in a "separate" personal fork.

If CI uses the secrets - it is not possible to test it in the fork properly.

[sergey-yaroslavtsev]

Did not work.
Since feeze_support was removed the frozen binaries again have the same issue...
The problem is that for my understanding it should be added to every entry point with protcetion of if __name__=__main__:
as it was in previous PR #1185

To avoid confusion i will make another PR - run dry-run there. We can substitute the DMG file tomorrow morning from there after test.

If you think it is a good idea one can delete the DMG file from current GitHub release.

[woutdenolf]

In this MR we never call freeze_support() and errors from the run_in_subprocess() call in safe_hdf5_group_keys() are captured and ignored. The error is caused by either multiprocessing cannot be imported (because not included in the frozen app) or the frozen app needs freeze_support (which we never call).

So the issue is that we need to do the same in the test, not that we need to call freeze_support() again because that was the alternative solution we rejected by merging this PR.

So instead of

try:
    import multiprocessing
except Exception:
    pass

class testHDF5Utils(unittest.TestCase):

    @unittest.skipIf("multiprocessing" not in sys.modules, "skipped multiprocessing missing")
    def testSegFault(self):
        self.assertEqual(_safe_cause_segfault(default=123), 123)

we should skip like safe_hdf5_group_keys() does

def _pass_through():
    return 0


def _cause_segfault():
    import ctypes

    i = ctypes.c_char(b"a")
    j = ctypes.pointer(i)
    c = 0
    while True:
        j[c] = b"a"
        c += 1

class testHDF5Utils(unittest.TestCase):

  def testSegFault(self):
    # Verify that run_in_subprocess can be used
    try:
        result = HDF5Utils.run_in_subprocess(_pass_through, default=123)
    except Exception:
        if not getattr(sys, "frozen", False):
            raise
        self.skipTest("multiprocessing does not work for the current frozen binary")
    self.assertEqual(result, 0)

    # Check that run_in_subprocess works as intended when the function segfaults
    result = HDF5Utils.run_in_subprocess(_cause_segfault, default=123)
    self.assertEqual(result, 123)