Closed editorialbot closed 2 months ago
Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.
For a list of things I can do to help you, just type:
@editorialbot commands
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
@editorialbot generate pdf
Software report:
github.com/AlDanial/cloc v 1.88 T=0.15 s (1118.4 files/s, 225670.0 lines/s)
-------------------------------------------------------------------------------
Language files blank comment code
-------------------------------------------------------------------------------
Rust 126 2800 5434 17166
Markdown 14 356 0 7114
R 16 168 120 480
YAML 5 78 99 368
TeX 1 2 0 239
TOML 7 32 28 124
PowerShell 1 14 29 28
JSON 1 0 0 19
C 1 2 2 4
-------------------------------------------------------------------------------
SUM: 172 3452 5712 25542
-------------------------------------------------------------------------------
gitinspector failed to run statistical information for the repository
Wordcount for paper.md
is 1636
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):
OK DOIs
- 10.18637/jss.v040.i08 is OK
- 10.1007/978-1-4614-6868-4 is OK
- 10.1080/00031305.2017.1375990 is OK
- 10.1038/d41586-020-03382-2 is OK
- 10.1007/978-981-15-1078-6_2 is OK
- 10.1093/bioinformatics/btv573 is OK
- 10.3384/ecp20176475 is OK
- 10.1101/2022.09.07.506908 is OK
MISSING DOIs
- None
INVALID DOIs
- None
@dccsillag, @luciorq,
Firstly, please create your checklist typing: @editorialbot generate my checklist
This is the review thread. Since the review process is interactive, you can always interact with the reviewers, the author, and the editor. You can open new issues in the target repository but please always mention the address of the review thread so we can keep tracking what is going on the outside our world. Do not hesitate to ask me anything.
Thank you in advance.
@dccsillag, @luciorq - Just pinging. How is your review going? The review process in JOSS is interactive, so whenever you have any issues or questions, please do not hesitate to keep a message post here. You can also open issues in the target repo with a mention link of the review page. Thank you in advance.
@dccsillag, @luciorq - Sorry for pinging, I've just seen the review has been inactive for a while. Could you please update your review status? Thank you.
Sorry for the delay! I should be able to advance the review a tad in the next few days.
@dccsillag, @luciorq - Dear reviewers, could you please update your status and inform us about how is your review going? Thank you in advance.
@dccsillag, @luciorq - Dear reviewers, I don't want to bother you too much, but, I should inform you that the expected duration of reviews is 40-60 days, of course more than those range is also okay, but I think the review will not likely finish in a reasonable time if this speed remains constant.
May I kindly ask you to find a proper time to continue your review?
I am so so sorry for bothering again and again.
Thank you in advance.
@dccsillag, @luciorq - We are currently slightly ahead of our usual review schedule. Would you mind providing an update on the status of your review? We would greatly appreciate it. Thank you in advance.
@CGMossa - I'm afraid we can't reach our reviewers; they may have turned off their notifications or could be very busy. Do we have alternative channels to reach them? Otherwise, we'll have to look for new reviewers.
I'm very sorry for my delay. I'm in the process of reviewing, but still haven't finished my review. Nevertheless, so that we can keep things flowing, I will try to post whatever I have of my review at most at the end of the weekend, and try to finish it during the next week.
Hello @jbytecode! I've attempted to ping @luciorq on our official ExtendR Discord Server, and I have not had a response yet. The message was sent on 04/16/2024 7:08 PM (CET), and the link to said message is here.
I would suggest giving the reviewers some more time to respond. At the very least, we may wait until @dccsillag has finished their review. Reviewing is hard, and especially a software package of this magnitude, so I trust that, there are a lot of effort going into processing the paper, and extendr overall.
@CGMossa - Thank you for reaching out to our reviewer. I also want to thank you for the suggestion. We always allocate enough time to reviewers and provide additional time when needed. However, the review process is interactive in JOSS, so it's expected that reviewers and authors interact on specific subjects continuously. Since I haven't seen progress and interaction (and at least a life signal) for a long while, it seems that the elapsed time hasn't been used for the submission, which, as you mentioned before, is a comprehensive undertaking.
Because editorial work and reviewing are voluntary, I tend to be quite lenient, but not receiving a response makes things difficult.
I'll wait to receive a sign of progress for a while.
Sorry for the excessive delay. I still haven't finished completely, but I feel like the remaining things will be relatively minor.
Extendr solves the relevant and important problem of making R bindings of Rust code (and vice versa), akin to what can be done for other languages already (e.g., for Python with PyO3). R is very prominent language that would benefit greatly from being able to easily write natively-compiled packages, so there is a great fit and I find ExtendR to be an excellent contribution to the community.
Moreover, Extendr seems fairly well-written and be the result of much care. I am confident that it is very well-suited for JOSS.
That said, I think there are two key factors to be improved prior to acceptance: documentation and it's behaviour on errors.
The documentation is in dire need of improvement. In particular, there seem to be bits of the documentation in various semi-scattered places, and with no cohesive link between them. For example, when I was getting started, I started at the GitHub README. Of course, what I really wanted from there were the docs and/or a quickstart guide. Looking for this, I fell into the docs for the development version of Extendr, following the link at the bottom of the README (which was the only reference to documentation that I could find). I do not know if there is a better version of these docs (the ones generated by rustdoc
), but its current presentation causes excessive confusion.
Perhaps the most prominent case of this is the sentence at the very top of the first page, stating "See Robj
for much of the content of this crate. Robj
provides a safe wrapper for the R object type." The docs should not point to Robj
at the first paragraph. After all, the basic usage of the crate is all about the #[extendr]
and extendr_module!
macros, and those (which are presented right in the following section) should come first. From the user's perspective, the Robj
API is not the most important part of the crate (even if from a developer perspective it is, because it's what implements the conversions). To a new user, it is a lot of effort to have to understand the Robj
API before diving into anything else, especially when for their first usage of Extendr Robj
will most likely be automatically used behind-the-scenes only.
That aside, I found it rather hard to understand how I should structure a Rust crate that provides R bindings. I ended up falling on the helloextendr repo, which I found rather clunky, to the degree that I was just wishing I could generaete them automatically. Fortunately, from there I ended up falling into the vignette at https://extendr.github.io/rextendr/articles/package.html, which explains how to do precisely that. My concern is (i) how hidden that vignette felt, and (ii) how scattered the docs are. So far, I've located docs in the form of GitHub READMEs, rustdoc-generated docs (for the development version), R docs, R vignettes (which also include things you would want to read even if you are just working on the Rust side), and probably a couple more. While I think it's understandable to have more than one location with documentation (especially considering Extendr bridges between languages), I think it's very important to try to (i) minimize their surface area, (ii) improve the presentation of each individually so that they are each reasonably self-sufficient, and (iii) better link between them all.
Dealing with computation that may fail is absolutely essential, and I understand this is something that is being worked on. However, I believe the current solutions must be improved. That said, it is possible that I've misunderstood something, so please correct me if I'm wrong.
Firstly, it seems that the easily accessible throw_r_error
function leaks memory due to not calling destructors. I agree that this function should be part of the API (seeing as it is a key part of the R API), but it absolutely should not be easy to use, as it essentially violates Rust's "guarantees" (I know, I know, leaking is technically allowed in safe Rust. But that does not mean we should not avoid it, if possible). Rust code should be "safe-by-default", ideally. Therefore, I strongly think that this function should be marked as unsafe
-- if the function is easy to use, then users will use it (especially as it has the appeal of being a mechanism to escape having to pipe Result<T,E>
s through!).
If there is a strong desire to have some functionality like it but without the unsafe
, then maybe an alternative version could be written that first panics, unwinding, and catching the panic and then calling the current throw_r_error
.
As for the returning Result<T,E>
option, as I understand, it makes the Rust side panic? I did not understand what truly happens in this case (and unfortunately have not been able to play around with it yet). But it does not seem sufficient, as the error does not get displayed to the R user?
Finally, there are the two experimental options. The list option sounds quite fine, though a bit more Rust-like then R-like (and thus maybe unfamiliar to R users). The error condition option also sounds quite nice, and is a bit more R-like, to the point that I wonder if it should not be the default. Could the authors clarify on why these are "experimental"?
That said, I wonder why there is not an option to return the T
straight-up if it's an Ok
, and throw an R error (stringify and then stop
?) with E
it's an Err
. After all, as far as I'm aware, that is what most R packages do, and so what an R user of a ExtendR-backed Rust package would expect.
Along the way, some relatively minor concerns:
test!
really be shown in the examples in the docs? While I get why it is (because then you can easily run the examples as tests), it is not standard Rust and so leads to some confusion when being used in reference pages.r!
macros for creating ExtendR objects? It feels to me like just .into()
(along with the associated Robj::from
) would have been sufficient and minimize the API surface, but I may have missed something (or perhaps underestimate the convenience of having a macro for this).Overall, I'd like to congratulate the authors for their great work so far on Extendr, and once again apologise for my tardiness.
Dear editor @jbytecode,
First, I'd like to say, that I didn't know that the review was supposed to be interactive. That really shifts things for me, as to how this process is supposed to be, and how it has been going thus far.
It has been a few weeks now, and @luciorq has yet to reply. I believe, we ought to attempt to find another reviewer, as there are no other way I can communicate with them at the moment.
I, and many of the contributors to extendr, have been working quite intensely on various things to improve the experience of using extendr. I do have a rebuttal to the points brought up here, https://github.com/openjournals/joss-reviews/issues/6394#issuecomment-2079541794, but I need a little more time for that. The changes have been proposed, reviewed, and merged ever since we received the review above. I just need time to write a cohesive response, and ensure I've covered all comments thoroughly.
Finally, I'd like to apologise to you, as I've let this process drag on like this. Several people that use extendr would like this paper published soon, but I think this process has helped us shape extendr into a better project overall.
@CGMossa - Please consider reviewing/changing/adding the suggestions of our reviewer. You can of course give answers and make comments on the suggestions. Those are the nautral steps of the review process of JOSS.
While you prepare a proposition and make changes, you can suggest me a new list of potential reviewers if you have. If you don't have one, I can search and assign a new reviewer.
Another option is to finish the first step review with @dccsillag and then wait for the response of @luciorq again. What do you think?
@luciorq - We really need your thoughts and review, otherwise, we will need to find and assign another reviewer. Could you please start your review and help us?
@kdpsingh, @alpaylan - One of our reviewers hasn't responded for a while. I can't reach them even in Twitter. A need a second reviewer for this submission. Would you willing to assist in reviewing this submission for JOSS?
@editorialbot remove @luciorq from reviewers
@luciorq removed from the reviewers list!
@kdpsingh, @alpaylan - One of our reviewers hasn't responded for a while. I can't reach them even in Twitter. A need a second reviewer for this submission. Would you willing to assist in reviewing this submission for JOSS?
Hi, I can be the second reviewer for this submission.
@editorialbot add @alpaylan as reviewer
@alpaylan added to the reviewers list!
@alpaylan - Thank you for accepting our invitation. Please, first type
@editorialbot generate my checklist
to generate your checklist. Here is the reviewing guidelines. You can always interact with the authors, the other reviewers, and the editor. You can also open pull requests and issues in the target repository. Please mention the review url in target repo so we can keep tracking what is going on out of our world.
Ping me anytime when you get into trouble. Thank you in advance
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Hi all! Pinging to see if we have any progress on this submission. Thank you in advance!
Dear Editor,
extendr contributors and I have started working on a general, User Guide, for using R, Rust and extendr. It is a bit of an on-going work, see here https://extendr.github.io/user-guide/, but it already addresses many of the concerns presented in the first review.
Since the submission of the paper, we've merged 39 PRs, with 6,381 additions and 3,358 deletions. (see this https://github.com/extendr/extendr/compare/paper...master)
This is only mentioning extendr crate, as the accompanying libR-sys
crate has also seen major changes.
Most of the changes has come about based on the feedback we've received here. We have not be too direct in our design choices, and mostly left it up to the users, to define certain behaviours, like the error handling. But since then, I've changed my mind, and now, I'm streamlining the design language of extendr. We've removed for instance the confusing #[extendr(use_try_from = true)]
option.
I still need a bit of time to implement and to figure out what the default behavior for error handling should be. Even if it will be simply adopting one of the existing strategies. And the content of the user guide ought to have a few more additions.
While I'm here, let me address a few (not all!) the points brought up in the review:
r!
is indeed unnecessary, as .into()
call is sufficient. However, the thing is with .into()
is that it lacks the
target type. Usually, type inference can do the job, but it isn't explicit.
Thus, we have extension trait that gives you .into_robj()
, which is explicit. And finally, r!
is then a convenience macro.test!
is a macro that embeds R into the test-runner. From R, you don't need to embed R, as it is present. But when
testing things in rust alone, for testing convenience, one may use test!
macro that ensures R is running. This macro
also provides an ability to use ?
within test functions. We used to have a lot of inquiries about why calls to R would crash in Rust, when people would call R functions outside R. Thus, we have the test!
macro everywhere to avoid this.
One more here, is that CRAN dislike code that embeds R in its packages. So, we encourage people to only use this
capability within test-code, that won't be part of the library, that's in R-packages.Finally, I'd like to say, that the paper contained a "Getting Started" segment that contains the minimally necessary functions from rextendr
to get started writing rust-powered R packages. I am convinced by the reviewer's opinion, that we should overhaul the first information presented to the user when entering https://extendr.github.io/. However, the current style of presenting API capabilities isn't different to what other packages in the same space do. Other contributors have also promised to take on this task, as they agree with the reviewer's take.
@CGMossa - Thank you very much for taking into account the feedback from our reviewer. Also, it is impossible not to appreciate your hard work. Please let us know when you are ready so that our reviewers can evaluate what has been done and can take further actions.
@alpaylan - It would be great to hear from you if we have progress in your side. Sorry for pinging and bothering. Thank you in advance.
Hi @jbytecode, sorry I'm unable to get to this due to other deadlines in the next 2 weeks. Would love to participate in the future if you still need input.
@alpaylan - It would be great to hear from you if we have progress in your side. Sorry for pinging and bothering. Thank you in advance.
Just to keep everyone up to date, I've been reading the paper/documentation/guidelines and slowly updating the checklist as I go through; but haven't been able to use the tool itself in sufficient detail to provide feedback.
@alpaylan - Thank you for the status update!
@alpaylan, @dccsillag, @CGMossa -May I have your thoughts and possibly an update from both sides on this submission?
Dear editor @jbytecode,
I've scheduled tomorrow to write an update to all the points raised by @dccsillag. A quick update from me, is that we've merged all the code changes that are necessary to address the issues mentioned. Today, I've called for testing on our discord server to all the users, because I want to release a new version of extendr, now that these extensive changes have landed.
So everything is progressing nicely.
Dear @jbytecode, @CGMossa,
Thank you for the patience. I checked all the boxes except the State of the field
in the submission checkbox, I think the package is well documented, as well as easy to use, solves an important problem that I see very important. The accompanying paper is also clear, and addresses the problems extendr
aims to solve. Additionally, I believe the comparisons of the paper with the rest of the literature needs further details, hence I did not check State of the field
box yet.
Expectations regarding comparison with the rest of the literature:
The savvy interface represents a distilled byproduct of ‘extendr’. However, these implementations differ from ‘extendr’ in that ‘extendr’ aims at providing an opinionated API, with a focus on an ergonomic API design inspired by features from Rcpp and cpp11.
I believe it is important to provide relevant context to demonstrate such comparative claims if possible. Such context can be given in the form of side by side examples of using both libraries and demonstrating what makes extendr more ergonomic compared to an alternative library, or detailing how the choices made by extendr
provides ergonomy and in what ways.
Additionally, if possible, having some quantitative measurements for comparing performances may also strengthen the paper, as was done in Rcpp paper that inspired this work.
Hello everyone!
(CC editor @jbytecode)
First, I will address the remaining points from @dccsillag, and then I'd like to address the comments from @alpaylan.
Yes! We need to have proper links and direction in the main README.md
at the extendr repository. I've filed that here
https://github.com/extendr/extendr/pull/795.
The overall point about having consolidated documentation on how to use extendr, that covers the R part, the Rust part, the extendR in Rust part is a very valid point. And we have taken first steps towards this by creating the User Guide over at https://extendr.github.io/user-guide/. The user-guide effort is on-going, but it already fulfils the purpose of unifying documentation. Most notably there is the step-by-step tutorial to take a rust crate, and make an R-package for it, see tutorial on porting a rust crate.
The reason for the User Guide, is because extendR has multiple projects, with many maintainers. libR-sys
is maintained by me, and people outside the extendR project, because it is useful to others. Then there is rextendr
which is on-going, and its development process is a bit separate from extendr-crates.
Therefore, we have established a new repository, with no pre-established owners, that then contribute to one unified documentation, instead of having to ping pong with three separate entities all the time.
We've been dealing with these concerns for years. There are certain limitations imposed by Rust and others by R (and CRAN compliance). For instance, we are tied to Rust 1.67, due to CRAN machines using out of support distributions (Fedora 39), etc. However, over the years, we have merged orthogonal ideas to error management in extendR to experiment and figure out what works, and what doesn't.
Thus, the documentation that @dccsillag pointed to was correct, up until now, where some of these issues have been rectified. We figured that this critical change merged in https://github.com/extendr/extendr/pull/781/files would resolve the leaking issue, even for the default behaviour. The claims will be removed in https://github.com/extendr/extendr/pull/784.
To me, this is as far as the error management effort can go for now. New versions of Rust must get installed on CRAN servers, before I can do more ergonomic changes to error handling. For now, we no longer leak by default, so that's good.
I've proposed a PR https://github.com/extendr/extendr/pull/784 to remove the performance claims present in the documentation. This also removes the snippet mentioned by @dccsillag
From the docs: "The minimal overhead of calling an extendr function is in the ballpark of 2-4us. Returning a condition or list increases the overhead to 4-8us. Checking & handling the result on R side will likely increase overall overhead to 8-16us, depending on how efficiently the result is handled." But the numbers here certainly depends on the specific hardware, right? Maybe it would be better to clarify the kind of hardware being considered here, or to speak in terms of relations (e.g., 40% slower) and not absolute numbers. (Yes, in the case of relations it's still a bit flawed, but much better than stating raw numbers.)
Overall, I don't think we make performance claims for the justification of extendR. And comparing the performance of extendR to other frameworks that support languages would pose a question as to what we are comparing to. Also, extendR attempts to provide the same functionality as Rcpp
, but the internal architecture of extendR is more similar to cpp11
, then Rcpp
. For instance, Robj
in Rust implement Clone
, which is not the case in either Rcpp
nor cpp11
.
This is not to say, that we aren't conscientious about performance. We recently merged https://github.com/extendr/extendr/pull/761, where the idea was, that C-strings have NUL
in them to mark the end of the string, and before we were iterating every string to find its length by detecting NUL
, and now we retrieve this information from the R runtime instead.
But I believe including performance benchmarks in the paper is out of scope of the paper, and mostly also outside the goals of extendR.
savvy
Thank you for your comments @alpaylan. We were hesitant as how to directly mention the difference between savvy
and extendr
.
I would refer to their own page for comparing extendR here and to quote:
extendr is great and ready to use, but it's not perfect in some points (e.g., error handling) and it's kind of stuck; extendr is too feature-rich and complex that no one can introduce a big breaking change easily. So, I needed to create a new, simple framework to experiment with. The main goal of savvy is to provide a simpler option other than extendr, not to be a complete alternative to extendr.
A direct statement about differences could be
savvy
does not have facilities to use Rust from an R session. Nor does it provide a knitr-engine for Rust code. It also does not provide automatically generate R wrappers to Rust functions, see https://yutannihilation.github.io/savvy/guide/key_ideas.html#no-implicit-conversions. Also, all savvy-functions must return aResult
. Futhermore,savvy
is not trying to be compliant with CRAN requirements.
I personally think that this is too direct of a comparison to fit in a paper, and it pivots the text a bit. savvy
is not opinionated, but extendr
is, seemed like a reasonable short sentence to state the difference.
Let me know what you'd advise on this matter.
@dccsillag - May I have your updates, please? Sorry for bothering.
@alpaylan - Could you please review the response that we got from our author and update your status?
@CGMossa - How is your work going? Is there anything new in your side? Could you please inform us?
Thank you all in advance!
Hello 👋 @jbytecode
All the PRs that I've mentioned in my reply above have been merged. I'm now back to regular development cycle of extendr, and waiting for a response on the paper review.
Hopefully, we can have this resolved soon, as I would like to push a new release of extendr together with the publication of this paper.
Dear @dccsillag and @alpaylan,
We have slightly exceeded the usual review timelines. Could we speed up a bit? May I kindly request that you to update your reviews in light of the latest developments?
I would be truly grateful if you could respond.
Thank you in advance.
@editorialbot generate pdf
:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:
Dear @jbytecode and @CGMossa,
As I've found the reasons to not place the comparison in the paper sufficient, I have checked the last box on the list.
Announce for everyone: I tried to reach our other reviewer via email. I hope to get a response soon.
I'm happy to see the recent advances in ExtendR's development, which I believe have already been significant improvements. I have a few minor comments still, but given the timing I consider myself essentially satisfied and hence checked the remaining items in my review list.
Regarding error management: thank you for the explanation, @CGMossa. Indeed, it's pretty frustrating to have to deal with these CRAN limitations, and I now better understand the context for the current state of things. I am very glad to see that there is no longer a leak by default, and the error management situation makes quite some sense to me now.
Regarding performance: I agree with all the raised points.
Finally, regarding the docs: the user guide is a great stride forward. It seems to be growing into a great material and certainly makes things a lot more clear. I do have some more comments on it, though:
That said, the current state of the docs and orientation around the ExtendR project's pages already seem quite an improvement from before. I also understand that these changes I've suggested take a while and quite a lot of effort, and given how long this review process has already dragged on along with my faith that the ExtendR team will continue to improve on these fronts, I consider myself satisfied.
Submitting author: !--author-handle-->@cgmossa<!--end-author-handle-- (Mossa Reimert) Repository: https://github.com/extendr/extendr Branch with paper.md (empty if default branch): paper Version: v0.6.0-joss Editor: !--editor-->@jbytecode<!--end-editor-- Reviewers: @dccsillag, @alpaylan Archive: 10.5281/zenodo.12584076
Status
Status badge code:
Reviewers and authors:
Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)
Reviewer instructions & questions
@dccsillag & @luciorq, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @jbytecode know.
✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨
Checklists
📝 Checklist for @dccsillag
📝 Checklist for @alpaylan