Skip to content

Comments

Fix parallel mode in Python 3.8+#83

Closed
rafecolton wants to merge 6 commits intoploxiln:masterfrom
Expensify:main
Closed

Fix parallel mode in Python 3.8+#83
rafecolton wants to merge 6 commits intoploxiln:masterfrom
Expensify:main

Conversation

@rafecolton
Copy link
Contributor

This fixes parallel mode in Python 3.8+ and closes #27. We have been using the change for production operations at Expensify and it works great ✅ Credit to @Stealthii for the code here

@ploxiln
Copy link
Owner

ploxiln commented Mar 14, 2025

Looks good, thanks.

Clearly it's been a long while since I touched this. I need to do some updates to the CI setup, get that working again. Then, please remove the version bump from this PR, and I'll do it separately after merging this.

@rafecolton
Copy link
Contributor Author

Removed the version bump. Happy to merge master once you get CI figured out, LMK.

@ploxiln
Copy link
Owner

ploxiln commented Mar 14, 2025

I merged the CI updates ... and dropping python2.7 support and more 😬 ... in #56

except ImportError:
import queue as Queue
from multiprocessing import Process
from multiprocessing.context import ForkProcess
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this import will fail on windows, which will cause fabric to fail to start (even if parallel isn't used)

# Attach exit codes now that we're all done & have joined all jobs
for job_name, exit_code in self._completed:
if isinstance(job, Process):
if isinstance(job, ForkProcess):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line doesn't really make sense here anyway: job is from the last iteration of for id, job in enumerate(self._running): above ...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, this weird state of affairs is from cdc597d

@ploxiln
Copy link
Owner

ploxiln commented Mar 16, 2025

closing in favor of #88 (it would be great if you can test that one!)

@ploxiln ploxiln closed this Mar 16, 2025
@rafecolton
Copy link
Contributor Author

Thank you. Seems to work well in basic testing on macOS. I'll get it merged into main of our fork and let you know if anyone encounters issues. We do use it occasionally on Linux as well, should have a chance to test that out this week.

@ploxiln
Copy link
Owner

ploxiln commented Mar 16, 2025

Thanks! I've tested parallel with #88 a bit on linux before merging it (but only with a few targets).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

@parallel doesn't work with Python 3.8

2 participants