Remove broken(?) mac/darwin specific code #3932
Conversation
by this patch, both should be unused TODO: write up a policy issue
but parsl has recently always used explicit Fork or Spawn processes, rather than the default (as part of that issue about not using Fork any more) so then the Darwin specific stuff is maybe just noise now
|
heads up @yadudoc that I have this in the pipeline as you made a bunch of the original functionality |
benclifford
changed the title
There was a problem hiding this comment.
Thanks @benclifford ! I tested this PR using code found in #3856 on MacOS 15 and found it no longer emits an error. Just in case, I also tested using Parsl 2025.09.01 on the same system and found the error does continue to emit there. Let me know if you have any questions and thank you for the fix!
|
@d33bs ok great. did you look in the monitoring data at all to see if it made sense? |
|
I'll begin merging this PR piece by piece as separate PRs until there is nothing left to merge here. |
Prior to this PR, each use of .qsize() was of the form q.qsize() != 0 which is equivalent to (not q.empty()) Queue.empty() is better supported across platforms than .qsize() This PR makes that substitution, meaning the broken (#3856) MacSafeQueue subclass will not be needed in this situation any more and the database manager will be able to work with a regular multiprocessing Queue even on Mac. This PR does not actually change the Queue type - it only reduces the functionality required by whichever queue is supplied. This is part of a series of PRs to remove that platform-specific functionality entirely - see PR #3932.
The radios do not make use of the .qsize() functionality of queues, and so th (broken, see #3856) platform-specific sized queue functionality is not necessary. This is part of a series of PRs to remove that platform-specific functionality entirely - see PR #3932. # Changed Behaviour This code is part of broken behaviour on Macs. The behaviour will be slightly better but still broken after this PR on that platform. Otherwise, the queue should not behave any differently. ## Type of change - Code maintenance/cleanup
Prior to this PR, each use of .qsize() was of the form ``` q.qsize() != 0 ``` which is equivalent to: ``` not q.empty() ``` Queue.empty() is better supported across platforms than .qsize() This PR makes that substitution, meaning the broken (#3856) MacSafeQueue subclass will not be needed in this situation any more and the database manager will be able to work with a regular multiprocessing Queue even on Mac. This PR does not actually change the Queue type - it only reduces the functionality required by whichever queue is supplied. This is part of a series of PRs to remove that platform-specific functionality entirely - see PR #3932. # Changed Behaviour The exit tests check if queues are empty or not. This PR assumes that the behaviour of `qsize() !=0` and `empty()` are the same. They might be subtly different. ## Type of change - Code maintenance/cleanup
|
@benclifford - apologies for the delay! The monitoring database created on the test MacOS environment looked to be sensible. I didn't do a deep dive on the data but all tables appeared and were populated with various records. Are there any tests I could run on the data to ensure integrity for the Parsl changes you're making? |
|
@d33bs the monitoring test suite isn't really set up for running on an environment that is very different from the GitHub Actions linux CI environment - so I'm wary of getting bogged down figuring out how dependencies should be installed / ignored / ... you could try this command which will run one specific monitoring test that uses htex: pytest 'parsl/tests/test_monitoring/test_basic.py::test_row_counts_base[htex_config]' --config local |
Since PR #3937, no .qsize() has been used in monitoring. This leaves MacSafeQueue, the Mac implementation of SizedQueue, unused. It will be removed in a subsequent PR. See PR #3932 for over-arching work # Changed Behaviour Monitoring should now work on Mac. # Fixes Fixes #3856 - hopefully. @d33bs has tested an approximately-equal change in #3932 on Mac, but that is not an auto-tested platform. ## Type of change - Bug fix
|
Relevant changes merged in other PRs. This PR originally removed: but that isn't directly relevant to removing the MacSafeQueue and is still one instance of fork mode multiprocessing in Parsl monitoring, to which the above code would apply. |
2 participants
Description
MacSafeQueue seems to be broken - on macs (issue #3856) , and as of Python 3.14, increasingly more so on Linux (where it is tested) - (PR #3924).
The only use of the features of MacSafeQueue is for queue length, and there only to determine if it is non-zero. This PR changes that to a test for the queue being empty, at every occurrence, meaning the same behaviour will be used on all platforms. I am hoping this fixes @d33b's problem on PR #3856.
This PR then removes the now-unused MacSafeQueue entirely, which should fix the test failure in PR #3924.
I saw a couple of other Darwin related changes which make multiprocessing use
forkby default across the whole Python environment. All of Parsl explicitly launches either a Fork process or a Spawn process now, so this default probably isn't used by Parsl on Darwin any more, and so the code is gratuitously messing with global state. This PR removes that.I intend to merge this PR in stages - basically the commits on this branch one by one.
Changed Behaviour
Monitoring might work on mac now. Asking @d33bs.
If you're used to Parsl forcing your own multiprocessing code to use
fork, that might break.Type of change