-
Notifications
You must be signed in to change notification settings - Fork 12
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: documentation and release pipelines #130
Conversation
@aseyboldt if you have any free time to spare, I'd love to get the remaining Rust issues finished so we can have a release with the Rust client in it |
Sorry @WardBrian for not getting back to this sooner, I still had a couple of other things I needed to catch up on. |
Looks like you beat me to it :-) |
About Rng and Send + Sync: I could be missing something, but I think they actually are Send and Sync. I don't think they contain any thread locals or so, do they? And I don't think there is any interior mutability either, as all functions that modify an rng take a mut reference. |
Ah fair enough, I hadn't considered that the only places you can use them require exclusivity anyway. In terms of remaining steps: |
I expanded some of the docs a bit, and made the argument description more consistent: https://github.com/aseyboldt/bridgestan/tree/rust-doc-improvements |
Thanks for that @aseyboldt. I will try to do a once-over on everything this week and merge this. |
I guess I need to enable a spell checker in my code editor :-) |
I think this is ready to go, I'd appreciate if both @aseyboldt and @roualdes could review the text before merging. I checked the output locally using |
These are `bindgen <https://docs.rs/bindgen>`__, which generates the Rust bindings from the C headers, | ||
and `libloading <https://docs.rs/libloading>`__, which provides easy dynamic library loading functionality. | ||
|
||
Care is taken to provide "safe" Rust wrappers so that users of the crate do not need to use the `unsafe` keyword. |
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.
You need an unsafe function to register a printing function, but I think that's probably not worth mentioning here, given that that's a pretty specialized use-case?
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.
Yes, I think that's the exception rather than the rule
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 looks good. Thanks to you both for putting in so much work on the Rust interface and the associated doc.
I'm marking request changes, solely based on the file extension in the link to the Rust example, not related to anything below this.
And maybe this is even a separate issue altogether, but I'm having with the tests. There are 4 failures on my machine after I run cargo test
and 9 tests passed. Here are the 4 failures
failures:
create_all_late_drop_fwd
create_all_parallel
create_all_serial
create_all_thread_serial
I also can't get the example to run. Though this is probably just my lack of Rust knowledge.
@roualdes what failures are you getting from those tests? We should probably provide the command to run the example somewhere, it is:
|
+1 to us including the I re-compiled all the test models with Without |
The thread safety guarantees the Rust interface gives (via the Rust type system) require STAN_THREADS. There should be a nice runtime message about this, but typing this out also makes me realize this is needs to be much more documented in the documentation for this interface |
Good catch. |
@aseyboldt those additions look great. Thanks for adding them. Will you merge your edits into this branch so we can accept this PR? Separately, we probably want a similar set of sentences for Julia. I'll open a new issue. |
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.
Looks great. Thanks!
@aseyboldt - can you imagine anything else to be done for Rust before we push a release? Otherwise I think we will try to do that today or tomorrow so that you can add a dependency in nuts-rs/nutpie |
@WardBrian Can't think of anything missing. |
Follow on from #105
CRATES_TOKEN
)