Skip to content
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

Rust Integration: Full find_min_cut Rust Integration #3

Merged
merged 55 commits into from
Dec 12, 2024

Conversation

ohjuny
Copy link
Collaborator

@ohjuny ohjuny commented Nov 9, 2024

This PR follows #1 (and replaces #2 ), which sets up pyo3 integration and replaces calls to nx.maximum_flow with a Rust implementation.

This PR replaces the entire call to find_min_cut with an equivalent Rust version. In particular, this means that the pre/post-processing of the capacity DAG is performed in Rust, and no Python/Rust context-switch is required during the entirety of find_min_cut.

Profiling across 9 workloads:

Additional notes: this PR initially aimed to also provide implementations for Operation and CostModel in Rust, so that annotate_capacity and reduce_durations could also be offloaded to Rust. However, there were some issues with using pyo3 to represent a trait-bound CostModel - there is a more detailed comment in the PR about this. The next steps for full Rust integration are outlined in Issue #4.

@ohjuny ohjuny marked this pull request as draft November 9, 2024 17:26
@ohjuny
Copy link
Collaborator Author

ohjuny commented Nov 15, 2024

(just noting down some things for myself)

Cost Model Issues
CostModel in Python is abstract base class - in Rust this is a trait. Operations have a cost model, which is generic over this trait. But pyo3 doesn't support passing in trait-bound objects. There are workarounds, in particular using Box<dyn ModelCost> where ModelCost is the trait - CostModel contains a dyn Box to ExponentialModel. But this makes the CostModel constructor weird, in that it needs to take in a dynamic type, which pyo3 doesn't support.

Edit: also found this dedicated trait bounds guide on the official pyo3 docs, didn't look through it deeply but could be helpful.

@ohjuny
Copy link
Collaborator Author

ohjuny commented Dec 10, 2024

The maturin-generated CI test for macos-12, x86_64 fails for some reason (wasn't failing in #1 ) so I commented it out in e489444. The only config I changed since #1 is that I updated pyo3 from 0.22.0 to 0.23.3 (because some other test was failing due to out-of-date pyo3). I think this should be fine but thought I'd leave a note.

@ohjuny ohjuny requested a review from jaywonchung December 10, 2024 23:07
@ohjuny ohjuny marked this pull request as ready for review December 10, 2024 23:08
@ohjuny ohjuny mentioned this pull request Dec 10, 2024
Copy link
Member

@jaywonchung jaywonchung left a comment

Choose a reason for hiding this comment

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

Beautiful. Nothing major. Thanks for your work!

src/lowtime_graph.rs Outdated Show resolved Hide resolved
src/lowtime_graph.rs Outdated Show resolved Hide resolved
src/lowtime_graph.rs Outdated Show resolved Hide resolved
src/lowtime_graph.rs Outdated Show resolved Hide resolved
src/lowtime_graph.rs Outdated Show resolved Hide resolved
src/lowtime_graph.rs Outdated Show resolved Hide resolved
src/phillips_dessouky.rs Show resolved Hide resolved
src/phillips_dessouky.rs Show resolved Hide resolved
src/phillips_dessouky.rs Outdated Show resolved Hide resolved
Copy link
Member

@jaywonchung jaywonchung left a comment

Choose a reason for hiding this comment

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

LGTM, very cool. Thanks @ohjuny!

@jaywonchung jaywonchung merged commit 4fcfe9d into lowtime-rust Dec 12, 2024
16 checks passed
@jaywonchung jaywonchung deleted the lowtime-rust-full-rust branch December 12, 2024 03:07
@jaywonchung jaywonchung restored the lowtime-rust-full-rust branch December 12, 2024 03:07
@jaywonchung
Copy link
Member

Just realized some commits mentioned in #2 might be in branches? @ohjuny Please clean up only the branches that can be deleted.

@ohjuny
Copy link
Collaborator Author

ohjuny commented Dec 12, 2024

Just realized some commits mentioned in #2 might be in branches? @ohjuny Please clean up only the branches that can be deleted.

Do you mean #4 ? If so, I think I can just keep this lowtime-rust-full-rust branch and delete the lowtime-rust-each-iter branch from #1 .

@jaywonchung
Copy link
Member

Oh, you're right. Sounds good!

@ohjuny ohjuny mentioned this pull request Dec 12, 2024
jaywonchung pushed a commit that referenced this pull request Dec 12, 2024
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.

2 participants