-
Notifications
You must be signed in to change notification settings - Fork 40
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
RcppEigen 0.3.4.0.0 preparations #103
Comments
I'll make sure the rstan-related ones get taken care of. @SteveBronder said that Stan is good for Eigen 3.4 in general, but some of those packages may have some old C++ code generated by Stan. |
With
Part of my
|
@zdebruine I see a compile failure (on Linux). As you are set up with |
Ah, the CRAN version of Eagle is actually ahead ( |
I noticed that
Even if |
@zdebruine Ah, what I mentioned was only concerned with the use of The problem of var_ans(long(indx[i]), 0) = ans1.dot(Mt.row(indx[i])); |
Hi sorry I'm running out the door right now so I'll comment in more detail tmrw, but I think the rstan on CRAN should be fine (using Stan 2.21) but Stan 2.21+ actually has some pretty major updates needed for Eigen 3.4. Early next week I can run Stan math 2.21's unit tests with Eigen 3.4 to see if anything breaks. If those pass then at least rstan should be good. When updating the develop version of the Stan math library to use Eigen 3.4 there are a few places we won't compile because we need to use some Eigen internals to understand if we are looking at a matrix or expression and one tricky set of decompositions which are failing our tests. We were going to do a release of rstan soon with Stan 2.26 and I think that would break with 3.4. But I think ^ is probably in the realm of Stan problems and not RcppEigen problems so maybe we could sort this out ourselves without needing anything |
@SteveBronder Thanks for the follow-up, but that's almost orthogonal to the question we are trying to answer here. Namely, who is going to step up and shepherd the ten known packages needing changes through making the changes. That set of packages does not include |
@eddelbuettel yes sorry that seemed a bit off topic, but I'm worried there are Stan functions not tested within |
The focus of this thread is to find a way to get the ten packages that at current do not compile under Eigen 3.4.0 to actually compile under Eigen 3.4.0. I cannot and will not do it alone, but I am willing help if others step up. So far nobody has, and if that doesn't change there won't be Eigen 3.4.0 at CRAN any time soon as they will not admit an update breaking ten existing packages. As a reminder, one can install RcppEigen 0.3.3.99.0 containing the snapshot in a single in dev environment, or Docker, or (with an extra argument) in libpath apart not hurting one's main system:
(This uses a convenience container that already has R and Rcpp, on an empty system you have to add the drat to |
I will try to take one or two, but can't promise any hard deadlines ... (I don't, personally, much care whether RcppEigen 3.4.0 happens, but I can see that it is a good thing generally, and I see the need to distribute the workload.) |
I feel the same. I still have a loooooong running Rcpp transition on my plate but this is also something that ought to get done, and ought to be doable. But it's a teaching term for me so my workload is a bit higher ... |
@eddelbuettel @bbolker I'm personally invested in seeing this happen (my RcppML package would make good use of Eigen 3.4), and happy to help, but I'm in the first week of my postdoc and a novice compared to all you experts. It'll be a few weeks before I can give this what it needs. |
@zdebruine I just wanted to jump in real quick to clear up some confusion. The versions are different because these are 2 different packages: |
I just tried to look into Eagle and failed on which majorly upset the template programming in Eigen and I have not found an easy way auto (via a temporary variable). Anybody? |
Ok, took another stab and I now have Eagle, eimpute, and fssemR sorted out. All were a "simple" case of needing a cast for one of the row or col indices (computed from another Eigen expression) and I will create PRs. ctsem is more of a mystery, maybe someone from team rstan can take a look (hi, @SteveBronder). |
Two quick updates:
|
I have completed one pass and managed to create seven out of nine patch sets. I had to bail on |
I will look at it. ctsem is one of the more complicated packges that uses
Stan.
…On Sun, Dec 5, 2021 at 5:41 PM Dirk Eddelbuettel ***@***.***> wrote:
I have completed one pass and managed to create seven out of nine patch
sets. I had to bail on ctsem (maybe @bgoodri <https://github.com/bgoodri>
and @SteveBronder <https://github.com/SteveBronder> can take a look?) as
well as on RMixtCompIO (anybody ?). Second looks would be appreciated as
would be gentle nags in due time to get the PRs and patches onto CRAN.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#103 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZ2XKVLNTWUV4E7L6PUFKTUPPTABANCNFSM5GZEESDQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
I'm just aware of this now, happy to try modify the stan code in ctsem but also not sure where to start. I'm available for any discussion / ideas @bgoodri ... |
@cdriveraus Yep, I didn't email you til yesterday so no need to feel guilty :) And as you can see other packages "need to move too". As |
I can confirm that the compilation error with the ctsem package and Eigen 3.4 is not due to anything specific to ctsem. The same error
(and similarly for
Stan definitely has implementations of https://github.com/stan-dev/math/blob/develop/stan/math/rev/core/std_isinf.hpp#L16 the problem is likely that those file are not getting included early enough. Stan's rev.hpp metaheader looks like
If that is correct, then this is definitely Stan's fault, but at the moment I don't know how to fix it without breaking our Eigen plugin. I also don't know how we got away with this under the previous versions of Eigen. @SteveBronder @andrjohns |
Maybe making (more explicit) use of #if __cplusplus > 201703L then you do not defone Edit I mixed up two parallel discussions that I am in -- sorry. This is not conditional on C++17 but "simply" on Eigen 3.4.0. You should still be able to test for it. |
I think the issue with I have been planning for ages to update StanHeaders on CRAN anyway, but doing so requires a lot of patching to make it backwards compatible with the CRAN packages that use it. We have achieved backwards compatibility on everything but Windows, but then got sidetracked because some package that has rstan in its Suggests wrote a bad test that crashed R and CRAN wants me to upload a new rstan that is more user-proof. Unfortunately, I think the StanHeaders that I want to get Windows-compatible and onto CRAN will expose another issue with how Stan solves a triangular system of equations under Eigen 3.4 that I will need to figure out a workaround for. TL;DR: If ctsem becomes the last package blocking RcppEigen, I'll upload a minor update to StanHeaders that just fixes the |
I have fixed that and merged the fixed into the fssemR on github now. Will work on submit a new version of package onto CRAN later. |
Sorry Stan has taken forever to sort this out. We just figured out the commit in the Eigen repo that caused our regression tests to fail. We're currently sorting out whether this is a Stan issue or Eigen issue. For anyone who wants to follow along the discussion is here |
See #123 -- help with RcppEigen towards Eigen 3.4.0 would be greatly appreciated. |
My impression is that Stan is good with either version of Eigen, but I am preparing to push another StanHeaders to CRAN, so I will run the reverse dependency checks with both versions of RcppEigen. |
StanHeaders 2.26 isn't compatible (would need this PR), but 2.32 is fine |
I've run the reverse-dependency checks locally, and there are now only a handful of packages which aren't compatible. The current status is summarised below:
|
Confirming. Running the patch 3.4.0 branch, I get a similar list on a full reverse-depends run. Without |
Here is a bit of > library(data.table)
> db <- data.table(tools::CRAN_package_db())
> pkgs <- c("approxOT", "eimpute", "FastJM", "GAGAs", "JMH", "nebula", "PartialNetwork", "RAINBOWR", "svines")
> db[Package %in% pkgs,.(Package,Version,Date,Published)][order(-Published)]
Package Version Date Published
<char> <char> <char> <char>
1: GAGAs 0.6.2 <NA> 2024-01-23
2: svines 0.2.3 <NA> 2024-01-18
3: approxOT 1.1 2024-01-15 2024-01-16
4: FastJM 1.4.1 2024-01-06 2024-01-09
5: RAINBOWR 0.1.33 <NA> 2023-09-12
6: PartialNetwork 1.0.2 2023-09-01 2023-08-22
7: nebula 1.4.2 2023-07-03 2023-07-05
8: JMH 1.0.2 2023-06-07 2023-06-15
9: eimpute 0.2.2 2022-10-22 2022-10-22
> leading to a shorter (yay!) list of open tickets: > pkgs <- c("approxOT", "eimpute", "FastJM", "JMH", "nebula", "PartialNetwork", "RAINBOWR")
> db[Package %in% pkgs,.(Package,Version,Date,Published)][order(-Published)]
Package Version Date Published
<char> <char> <char> <char>
1: approxOT 1.1 2024-01-15 2024-01-16
2: FastJM 1.4.1 2024-01-06 2024-01-09
3: RAINBOWR 0.1.33 <NA> 2023-09-12
4: PartialNetwork 1.0.2 2023-09-01 2023-08-22
5: nebula 1.4.2 2023-07-03 2023-07-05
6: JMH 1.0.2 2023-06-07 2023-06-15
7: eimpute 0.2.2 2022-10-22 2022-10-22
> |
One shorter set of failures on another rev.dep run: eimpute, FastJM, JMH, nebula, PartialNetwork, RAINBOWR. > library(data.table)
data.table 1.15.0 using 6 threads (see ?getDTthreads). Latest news: r-datatable.com
> db <- data.table(tools::CRAN_package_db())
> pkgs <- c("eimpute", "FastJM", "JMH", "nebula", "PartialNetwork", "RAINBOWR")
> db[Package %in% pkgs,.(Package,Version,Date,Published)][order(-Published)]
Package Version Date Published
<char> <char> <char> <char>
1: FastJM 1.4.1 2024-01-06 2024-01-09
2: RAINBOWR 0.1.33 <NA> 2023-09-12
3: PartialNetwork 1.0.2 2023-09-01 2023-08-22
4: nebula 1.4.2 2023-07-03 2023-07-05
5: JMH 1.0.2 2023-06-07 2023-06-15
6: eimpute 0.2.2 2022-10-22 2022-10-22
> |
Ran another reverse dependency check after making a small change in the main branch. Unchanged: same six packages failing. I am now going to send an email to the five distinct maintainers of these packages, reminding them of the PRs kindly prepared by @andrjohns and proposing an actual release two weeks from now on Feb 28. |
> library(data.table)
data.table 1.15.0 using 6 threads (see ?getDTthreads). Latest news: r-datatable.com
> db <- data.table(tools::CRAN_package_db())
> pkgs <- c("eimpute", "FastJM", "JMH", "PartialNetwork", "RAINBOWR")
> db[Package %in% pkgs,.(Package,Version,Date,Published)][order(-Published)]
Package Version Date Published
<char> <char> <char> <char>
1: FastJM 1.4.1 2024-01-06 2024-01-09
2: RAINBOWR 0.1.33 <NA> 2023-09-12
3: PartialNetwork 1.0.2 2023-09-01 2023-08-22
4: JMH 1.0.2 2023-06-07 2023-06-15
5: eimpute 0.2.2 2022-10-22 2022-10-22
> |
I suspect that the email of the original maintainer of |
Agreed that that is problematic, but CRAN has a way of taking care of it. I have seen a number of packages from the Rcpp nexus disappear in the last few weeks (often over simple I am really thankful for the work you and @andrjohns did: it is time we get to 3.4.0. If only five packages are in the way among well over 400 using RcppEigen then it is worth proceeding. That said, let's reach out as best as we can. That several of the PRs were merged is good too. |
@yixuan Now confirmed as I got email from Zhe Gao, now at USTC. |
Yeah I also received an update email from Zhe. Seems we have moved one step forward! |
Yes indeed. The possible 'shrapnel' is now under 1% -- 4 out of over 400 packages. And they all have merged. This will, after all, work out and quite smoothly. Nice when patience pays off. Thanks again for your persistent help all along! |
And So now at > library(data.table)
data.table 1.15.0 using 6 threads (see ?getDTthreads). Latest news: r-datatable.com
> db <- data.table(tools::CRAN_package_db())
> pkgs <- c("FastJM", "JMH", "PartialNetwork", "RAINBOWR")
> db[Package %in% pkgs,.(Package,Version,Date,Published)][order(-Published)]
Package Version Date Published
<char> <char> <char> <char>
1: FastJM 1.4.1 2024-01-06 2024-01-09
2: RAINBOWR 0.1.33 <NA> 2023-09-12
3: PartialNetwork 1.0.2 2023-09-01 2023-08-22
4: JMH 1.0.2 2023-06-07 2023-06-15
> |
Now JMH is on CRAN too so we are down to three: > library(data.table)
data.table 1.15.0 using 6 threads (see ?getDTthreads). Latest news: r-datatable.com
> db <- data.table(tools::CRAN_package_db())
> pkgs <- c("FastJM", "PartialNetwork", "RAINBOWR")
> db[Package %in% pkgs,.(Package,Version,Date,Published)][order(-Published)]
Package Version Date Published
<char> <char> <char> <char>
1: FastJM 1.4.1 2024-01-06 2024-01-09
2: RAINBOWR 0.1.33 <NA> 2023-09-12
3: PartialNetwork 1.0.2 2023-09-01 2023-08-22
>
|
It has been shipped to CRAN. 🤞 |
Yay yay yay! Huge huge thank you to all who help, especially @yixuan @andrjohns @jaganmn ! |
And |
And for completeness I just saw |
NB: This (old) initial entry is outdated. Best to scroll to the bottom. As of late Feb 2024 we are down to three affected packages out of well over 400.
So with #102 (thanks again, @yixuan, for preparing it) we have a release candidate for RcppEigen 0.3.4.0.0. It is currently running a first pass of reverse depends, and a few packages do indeed go belly up.
I suggest we pool resources between myself, @zdebruine, @yixuan, @bbolker ... and whoever wants and can help! Maybe some of the
rstan
folks want to pitch in?While the reverse depends run is still 'cooking' we have failures of these packages (and I will edit/complete this as it finishes)The run is now done (and fully summarized in this commit). We should focus on these ten failures out of 329 packages tested:RAINBOWR(unrelated to RcppEigen, see below)Let's not despair---as I write this 191 have passes, or 20x more than those few failures. (I also skipped three)It would best if volunteers could step forward and pick package to adopt for a fix. A release candidate version of RcppEigen is in the Rcpp drat repo and be installed via
install.packages("RcppEigen", repo="https://RcppCore.github.io/drat")
(and you may have to settype="source"
, I generally don't on Linux).If you adopt a package, comment below and edit your comment as you move along, eventually (ideally) showing a PR to the package you adopted. We can get this done---it is a matter of effectively parcelling out the grunt work. Let's do this.
The error summary (committed as usual to the rcpp-logs repo) is
I have skipped the package that passed once its dependencies were added and two with apparently unrelated test error. That leaves ten which failed to install under 0.3.3.99.0 which is surely something we need to address if we want the package on CRAN.
The text was updated successfully, but these errors were encountered: