Skip to content

Decouple Python bindings from Rust core and add Rust convenience API#38

Open
StoatPower wants to merge 5 commits into
SuperDARNCanada:developfrom
StoatPower:feature-gate-python-bindings
Open

Decouple Python bindings from Rust core and add Rust convenience API#38
StoatPower wants to merge 5 commits into
SuperDARNCanada:developfrom
StoatPower:feature-gate-python-bindings

Conversation

@StoatPower

Copy link
Copy Markdown

Hi @RemingtonRohel 👋

I'm currently working with the Virginia Tech SuperDARN lab and have been exploring use of dmap from Elixir via Rustler without introducing a Python dependency (wip -> darn-dmap).

That requirement motivated this PR which separates the Python bindings from the core Rust functionality so it can be consumed independently. The Python functionality is preserved while being feature gated behind the flag --features python, passable to both cargo and maturin.

Also added are top-level Rust convenience functions generated via macro and following the existing dmap conventions. Unit tests are added that assert parity between the convenience function API and the internal functionality per record type.

I also updated the MSRV to rust-version = "1.85.1" to match the current dependency requirements. While testing, versions older than 1.85.1 surfaced build failures due to dependency drift.

All tests are passing for both Rust (cargo test) and Python (cargo test --features python), and the PR is fully up-to-date with your most recent work on version 0.8.0.

I'd be happy to make any changes or discuss alternative approaches if you have a preferred direction for the crate.

Thanks for your work on the project!

~ Matt

…neration code is decoupled from rust core; include it with feature flag ; goal is to have a clean rust core api for dmap that can more cleanly be consumed/wrapped by languages other than just python

added comments for the rust read api

added unit tests for Rust's read api
The previous rust-version of 1.63.0 no longer builds with the
current dependency graph. Testing identified dependencies requiring
newer toolchains and edition 2024 support.

Keep the crate on edition 2021 while updating the minimum supported
Rust version to 1.85.1.
@RemingtonRohel

Copy link
Copy Markdown
Contributor

Hi @StoatPower, this is a very welcome contribution, thank you!

A few notes for discussion:

  1. We need the line features = ["python"] under the tool.maturin table in pyproject.toml, so the Python bindings will be properly generated by the CI.
  2. The macros you made in lib.rs for oneshot reading/writing functions for the Rust API aren't necessary in my opinion, they are basically just shims over the functions defined in the Record trait that the end user already has access to. I think it would be cleaner to remove them, but I'm open to your opinion on this as you're the first user of the Rust API to my knowledge.

Again, thanks for all the work, I very much appreciate it!

@StoatPower

Copy link
Copy Markdown
Author

Hi again and thanks so much for taking the time to look over the PR and provide your insight!

I fully agree with your point 2) that the top-level convenience functions aren't strictly necessary. If I'm being honest, I've always found it a battle to decide when and if to introduce convenience functions to any library or code base. At what point does the often subtle ergonomic boost outweigh the added maintenance concern? 🤔

My main argument for keeping them is not that they add new capability, but that they provide a single, semantic entry point for the primary API surface. From a consumer perspective, that can reduce friction and improve discoverability, while leaving some flexibility for the public API to evolve separately from the underlying implementation of each format.

A few examples I can think of:

  • If a future version adds another DMAP-backed format, crate-level functions make that immediately visible from autocomplete when typing dmap::read_.... The trait implementation would also be discoverable, of course, but only once the user already knows which concrete type or module to look for.
  • Today, these functions are mostly one-shot shims, as you said. But similar to the Python API, a consolidated consumer-facing API gives room for more intentional naming and ergonomics over time without tying those decisions directly to the Record trait methods.
  • Similarly, at the moment, all supported formats expose the same basic functionality through Record. If future formats diverge, crate-level functions could preserve a consistent high-level API while allowing the lower-level implementations to vary.

One other thing that influenced me was that the crate already exposes format-specific try_write_* helpers generated by write_rust!. I don't think symmetry alone is a sufficient reason to keep the read helpers, but it did make the API feel more coherent to me to have analogous read and write entry points from the crate root.

All that said, I should acknowledge that I'm not a native Rustacean and may be bringing expectations from other ecosystems with me. It's also entirely possible the benefits of the convenience API aren't significant enough to justify the added maintenance burden.

Ultimately, I'm more than happy to defer to your direction here. The library I'm working on that consumes the Rust API will not suffer greatly without the convenience layer.

Oh, and regards to point 1), I'll patch that in later today!

Thank you!

@RemingtonRohel

Copy link
Copy Markdown
Contributor

I think I'm going to stand by my opinion at this time, but you raise a lot of good points and I think if I expected this to be a more heavily used crate I would be swayed. As it stands, I am not anticipating very many users of the Rust API, and the docs page (e.g.) shows all the Record type impls and functions anyways, and would be where a user should go for usage guidance anyways (I've always found docs.rs pages much more helpful than IDE autocomplete).
I also worry about maintainability, Rust developers in the SuperDARN community are scarce and if someone else needs to pick this up my opinion is that less code to maintain is better.

I wouldn't claim to be an expert by any means on this, this was my first Rust project to learn the language and it was a hard fought battle to get it as tidy as it is along the way.

Thanks for patching in the pyproject change, if you don't mind modifying lib.rs as well then we can merge this in and I'll draft a new release probably next week.

@StoatPower

Copy link
Copy Markdown
Author

Absolutely not a problem; I'll have the updates pushed momentarily!

…ts sibling . Updated the Python bindings to call it as appropriate.
@StoatPower

Copy link
Copy Markdown
Author

@RemingtonRohel

I initially left the write_rust macro in lib.rs when splitting out the python functionality simply because it didn't have python dependencies, and since I was mostly focused on the read-level functionality, I didn't give it a ton of thought at the time.

Once I took a closer look it felt a bit out-of-place all alone there in lib.rs, and I realized the functionality fit more naturally alongside its sibling write_to_file on the Record trait. That keeps the Rust API centered around Record while allowing the Python bindings to simply consume that API rather than relying on separate crate-level helper functions.

I've pushing another commit that removes it as a macro from lib.rs and moves it into Record. I've accordingly updated the calls to it from python_bindings.rs. I think it cleans things up pretty well and all tests are green and happy.

If you have a chance, I'd appreciate your thoughts on it. It feels like a better fit with the direction we discussed, but I'd be interested to hear whether you think it lands in the right place or if I'm missing something.

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