Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement multithreaded stochastic swap in rust #7658
Implement multithreaded stochastic swap in rust #7658
Changes from all commits
9d9c2dc
7af1c43
2f8de01
6da1b7b
3cf6e04
f838ff6
780f327
35fba96
d72d6c5
0b6e4d7
a9a879d
57790c8
ea23eae
18a8b56
6a63d11
873de02
2d9aae0
76167c7
2685c01
610b306
c510764
b7607c1
5e31fb2
cfc8fcb
d726101
fb70158
25187b8
b2a0536
f2737aa
2588837
93ded7f
e7a6b93
91f0da9
6da2a3a
3d1d561
25b2747
fc109d2
eaf92f2
b318a2a
4c4c9d4
73296a4
eb69288
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This feels like a potentially large change to CI - is it completely necessary? If it's only necessary for the
verify_parallel_map
script, perhaps we could just run that in a separate CI step?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.
It was necessary when I wrote this change, see the commit message on: 93ded7f for the details. But, I updated the logic here in 3d1d561 to not run multithreaded code when we're in
parallel_map()
so I think we might be able to get away without adding this. That being said doing this made CI noticeably faster because we're not running 2x processes everywhere anymore.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.
This is good enough for me - my only minor concern is that we might fail to test some code that should run in parallel now, but I think we should be able to override that anyway if necessary within the test suite.
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.
Yeah, we can just copy the model I used in
verify_parallel_map
to override the env variable, or alternatively we can always explicitly add on to that script too for things we want to ensure are run in parallel.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.
This feels like a potential shortcoming of PyO3, or maybe it's general Python extensions. Either way, this seems like a fine workaround.
The second sentence in the comment is hard to understand - I think maybe it's just missing a full stop at the end of line 24 and a new sentence starting on line 25?
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.
It's a limitation of compiled extensions in general. Python's importer doesn't know how to search for submodules in a compiled extension. It's just extra annoying from rust because it doesn't give you a way to expose multiple binary libs from a single library project as it expects people to use workspaces to do multiple libraries so we're stuck either doing this or creating a separate workspace (which means a separate dir with a standalone cargo.toml) which would have a lot of disadvantages.
This file was deleted.
This file was deleted.