♻️ Refactor setup.py for improved build process #435
Conversation
- replace `os.path` to `pathlib.Path` - formatted by `ruff` - add multiprocessing method `"spawn"` to intend windows build.
|
seems like the Python 3.7 test became obsolete for the latest Ubuntu CI runner. |
There was a problem hiding this comment.
Thanks for the PR. I'll let @CnlPepper give his say on this, but my general impression is that this produces a fair bit of code churn without significant practical benefits. Unless there is a genuine problem that needs to be solved with setup.py it's hard to see why it needs so many changes. But that may just be me.
Also, please don't use emojis in commit messages. They don't render properly in the git pager in most terminals. Keep to plain text.
|
|
||
| multiprocessing.set_start_method('fork') | ||
|
|
||
| if os.name == "nt": |
There was a problem hiding this comment.
Raysect 0.8.x does not support Windows. The multiprocessing paradigm it uses relies on the Linux kernel's copy-on-write when forking processes, and this is not something Windows supports. The fix is to use a new parallel render engine which doesn't require fork's COW semantics, and this is not a small project. Adjusting setup.py to support building on Windows may give Windows users the false impression that they should expect Raysect to work well on this platform.
There was a problem hiding this comment.
Thank you for your review!
Is the incompatibility for Windows tied to all multiprocessing methods? If so, can I also fix this issue by modifying the workflow.py module?
(I guess things are not so simple...)
There was a problem hiding this comment.
Changing "fork" to "spawn" in MulticoreEngine will cause local copies of the entire scene graph, including all its data, to be created in child processes. Meshes used in Raysect/Cherab scenes can take up several gigabytes. With dozens of parallel processes, this can lead to out of memory problem, not to mention possible performance issues. The scene graph must be shared between all the processes instead of copying. This is currently achieved using the POSIX fork() Copy-on-Write semantics. I don't know of any other way to do this in pure Python. I think the best way to use Raysect on Windows is via WSL2.
There was a problem hiding this comment.
Python 3.13 added an experimental free-threading build. In the future, multithreading may replace multiprocessing in Raysect's MulticoreEngine.
There was a problem hiding this comment.
If Matt and I ever get time to return to Raysect development (its not impossible, just impractical for now) I'll like to experiment with the new threading model.
There was a problem hiding this comment.
@vsnever, thank you for your comment! "fork" seems to use a shared memory scheme, doesn't it?
The GIL-free threading is very attractive and could be an option in the future.
@CnlPepper Thank you for commenting! I can't wait for you to come back as a developer😢
There was a problem hiding this comment.
I changed the "spawn" setting in the if case to raise OSError to throw an error if the OS is Windows.
setup.py
Outdated
| ], | ||
| install_requires=['numpy', 'matplotlib'], | ||
| install_requires=["numpy", "matplotlib"], |
There was a problem hiding this comment.
In pyproject.toml we use oldest-supported-numpy. The project is thus built against numpy 1.x for all versions of Python where that is supported, and means in turn that numpy<2 is required at runtime.
There was a problem hiding this comment.
Yes, we should keep numpy<2.0 in runtime.
However, when I specify numpy > 2.0 in build time, raysect's unit tests seem to be succeeded according to the conda building process.
Needless to say, the unit test does not seem to cover all raysect's functionalities.
There was a problem hiding this comment.
Also, some demo scripts were successfully executed even in numpy==2.1.2.
|
Sorry for the abuse of emojis in any commit. As @jacklovell mentioned, this PR is not a mandatory change, but it will make codes readable by
|
|
Thanks for the PR, however as Jack said I neither see the necessity nor do we support windows for reasons already identified. |
|
Sorry hit the wrong button. |
|
What is the issue you are trying to solve? |
|
@CnlPepper I am trying to use |
|
Why would you change known working code? The current pathing is not deprecated and it is already proven to function. Pythons packaging systems are fickle and difficult to debug across the range of tooling people use to install packages. Wanting to "modernize* the code ia not a sufficient justification to change the code. i'm sorry, but I'm not merging this. All I'll be doing is creating a potential maintenace headache for myself. |
|
I agree that the Python packaging process is drastically changing, even though it was born decades ago. |
os.pathtopathlib.Pathruff"spawn"to intend windows build.