-
-
Notifications
You must be signed in to change notification settings - Fork 652
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow processes to occupy more than one slot in the execution semaphore #21960
base: main
Are you sure you want to change the base?
Allow processes to occupy more than one slot in the execution semaphore #21960
Conversation
Sorry @chris-smith-zocdoc , I think this slipped below the fold. Looking now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a great approach! Once it's polished up and ready for review, you can assign me and @tdyas . Thanks for your patience!
Exclusive, | ||
} | ||
|
||
// TODO Unclear why I need this or when its invoked, compile error for CommandSpec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to figure this out...
let py_type = conc.get_type(); | ||
let py_name = py_type.name().unwrap(); | ||
let type_name = py_name.to_str().unwrap(); | ||
match type_name { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clunky, but I don't see a better way to model a rust enum in Python, so this is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current pass at the python api feels clunky to use too though, I could use some perspective there in how we'd want plugin authors to define this (and eventually how it could trickle down into the fields of targets if necessary)
fixes #21626
Draft for early feedback on the approach.
I was able to manually test that this appears to work from the plugin side, though I plan on adding tests here once the design is agreed upon.
Slack conversation https://pantsbuild.slack.com/archives/C0D7TNJHL/p1738988769732369?thread_ts=1738988769.732369&cid=C0D7TNJHL