[core] Downgrade vendored setproctitle to 1.2.3#60185
[core] Downgrade vendored setproctitle to 1.2.3#60185edoakes merged 1 commit intoray-project:masterfrom
Conversation
All versions of setproctitle after 1.2.3 seem to introduce significant expense when forking. This in turn results in very slow or crashing runs of ray jobs on macOs, particularly when spawning many jobs. Looking at the code, it seems as if versions after 1.2.3 make quite a lot of changes on Darwin in order to make the process renames work properly with Activity monitor and other MacOs utilities. This... is not 'especially' important to Ray. So downgrading doesn't cost us too much. Though of course it would have been vastly preferable to rely on a non-vendored latest version, but latest versions seem to have this issue. So we can downgrade for now and come back later potentially. Signed-off-by: ZacAttack <zac@anyscale.com>
There was a problem hiding this comment.
Code Review
This pull request downgrades the vendored setproctitle library to version 1.2.3 to address a performance issue on macOS. The changes correctly replace the newer, problematic macOS-specific implementation with the older, more traditional argv clobbering method. The build files and documentation are updated accordingly to reflect this downgrade. The changes are logical and well-aligned with the stated goal. I've identified a few minor issues in comments and debug messages that could be addressed to improve code quality.
|
For posterity (since I had to put the pieces together): in the original PR, one of the motivations was to avoid side effects on import. This was introduced upstream in: dvarrazzo/py-setproctitle#114. Because we're only vendoring the C code and have our own Cython bindings, that behavior is unchanged in this patch. |
israbbani
left a comment
There was a problem hiding this comment.
Can we capture context of your investigation and the original user reported bug in the PR description?
|
Thanks for the fix! I ran the repro script on macOS and can confirm the regression is resolved and performance is back to normal. |
**Problem** All versions of setproctitle after 1.2.3 seem to introduce significant expense when forking. This in turn results in very slow or crashing runs of ray jobs on macOs, particularly when spawning many jobs. **Solution** Looking at the code, it seems as if versions after 1.2.3 make quite a lot of changes on Darwin in order to make the process renames work properly with Activity monitor and other MacOs utilities. This... is not 'especially' important to Ray. So downgrading doesn't cost us too much. Though of course it would have been vastly preferable to rely on a non-vendored latest version, but latest versions seem to have this issue. So we can downgrade for now and come back later potentially. ## Related issues fixes ray-project#59663 **Historic Context** ray-project#53471 vendored the dependency and made a slight logic tweak in the cython binding in setproctitle.pxi. This had the benefit of fixing the cmdline parse issue described in the PR but had the downside of upgrading the library version (which now included a set of Darwin tweaks which leads to the slowdown). After this PR, the state will be that the vendored version is now old enough to not contain the activity monitor tweaks for Darwin, as well as having the changes in setproctitle.pxi. Signed-off-by: ZacAttack <zac@anyscale.com> Signed-off-by: jeffery4011 <jefferyshen1015@gmail.com>
…60185)" (ray-project#60361) Signed-off-by: joshlee <joshlee@anyscale.com> Signed-off-by: jinbum-kim <jinbum9958@gmail.com>
…60185)" (ray-project#60361) Signed-off-by: joshlee <joshlee@anyscale.com> Signed-off-by: 400Ping <jiekaichang@apache.org>
**Problem** All versions of setproctitle after 1.2.3 seem to introduce significant expense when forking. This in turn results in very slow or crashing runs of ray jobs on macOs, particularly when spawning many jobs. **Solution** Looking at the code, it seems as if versions after 1.2.3 make quite a lot of changes on Darwin in order to make the process renames work properly with Activity monitor and other MacOs utilities. This... is not 'especially' important to Ray. So downgrading doesn't cost us too much. Though of course it would have been vastly preferable to rely on a non-vendored latest version, but latest versions seem to have this issue. So we can downgrade for now and come back later potentially. ## Related issues fixes ray-project#59663 **Historic Context** ray-project#53471 vendored the dependency and made a slight logic tweak in the cython binding in setproctitle.pxi. This had the benefit of fixing the cmdline parse issue described in the PR but had the downside of upgrading the library version (which now included a set of Darwin tweaks which leads to the slowdown). After this PR, the state will be that the vendored version is now old enough to not contain the activity monitor tweaks for Darwin, as well as having the changes in setproctitle.pxi. Signed-off-by: ZacAttack <zac@anyscale.com>
…60185)" (ray-project#60361) Signed-off-by: joshlee <joshlee@anyscale.com>
**Problem** All versions of setproctitle after 1.2.3 seem to introduce significant expense when forking. This in turn results in very slow or crashing runs of ray jobs on macOs, particularly when spawning many jobs. **Solution** Looking at the code, it seems as if versions after 1.2.3 make quite a lot of changes on Darwin in order to make the process renames work properly with Activity monitor and other MacOs utilities. This... is not 'especially' important to Ray. So downgrading doesn't cost us too much. Though of course it would have been vastly preferable to rely on a non-vendored latest version, but latest versions seem to have this issue. So we can downgrade for now and come back later potentially. ## Related issues fixes ray-project#59663 **Historic Context** ray-project#53471 vendored the dependency and made a slight logic tweak in the cython binding in setproctitle.pxi. This had the benefit of fixing the cmdline parse issue described in the PR but had the downside of upgrading the library version (which now included a set of Darwin tweaks which leads to the slowdown). After this PR, the state will be that the vendored version is now old enough to not contain the activity monitor tweaks for Darwin, as well as having the changes in setproctitle.pxi. Signed-off-by: ZacAttack <zac@anyscale.com> Signed-off-by: peterxcli <peterxcli@gmail.com>
…60185)" (ray-project#60361) Signed-off-by: joshlee <joshlee@anyscale.com> Signed-off-by: peterxcli <peterxcli@gmail.com>
**Problem** All versions of setproctitle after 1.2.3 seem to introduce significant expense when forking. This in turn results in very slow or crashing runs of ray jobs on macOs, particularly when spawning many jobs. **Solution** Looking at the code, it seems as if versions after 1.2.3 make quite a lot of changes on Darwin in order to make the process renames work properly with Activity monitor and other MacOs utilities. This... is not 'especially' important to Ray. So downgrading doesn't cost us too much. Though of course it would have been vastly preferable to rely on a non-vendored latest version, but latest versions seem to have this issue. So we can downgrade for now and come back later potentially. ## Related issues fixes ray-project#59663 **Historic Context** ray-project#53471 vendored the dependency and made a slight logic tweak in the cython binding in setproctitle.pxi. This had the benefit of fixing the cmdline parse issue described in the PR but had the downside of upgrading the library version (which now included a set of Darwin tweaks which leads to the slowdown). After this PR, the state will be that the vendored version is now old enough to not contain the activity monitor tweaks for Darwin, as well as having the changes in setproctitle.pxi. Signed-off-by: ZacAttack <zac@anyscale.com> Signed-off-by: peterxcli <peterxcli@gmail.com>
…60185)" (ray-project#60361) Signed-off-by: joshlee <joshlee@anyscale.com> Signed-off-by: peterxcli <peterxcli@gmail.com>
Description
Problem
All versions of setproctitle after 1.2.3 seem to introduce significant expense when forking. This in turn results in very slow or crashing runs of ray jobs on macOs, particularly when spawning many jobs.
Solution
Looking at the code, it seems as if versions after 1.2.3 make quite a lot of changes on Darwin in order to make the process renames work properly with Activity monitor and other MacOs utilities. This... is not 'especially' important to Ray. So downgrading doesn't cost us too much. Though of course it would have been vastly preferable to rely on a non-vendored latest version, but latest versions seem to have this issue. So we can downgrade for now and come back later potentially.
Related issues
fixes #59663
Historic Context
#53471 vendored the dependency and made a slight logic tweak in the cython binding in setproctitle.pxi. This had the benefit of fixing the cmdline parse issue described in the PR but had the downside of upgrading the library version (which now included a set of Darwin tweaks which leads to the slowdown). After this PR, the state will be that the vendored version is now old enough to not contain the activity monitor tweaks for Darwin, as well as having the changes in setproctitle.pxi.
Additional information