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

Release checks #3211

Closed
mattdowle opened this issue Dec 13, 2018 · 25 comments
Closed

Release checks #3211

mattdowle opened this issue Dec 13, 2018 · 25 comments
Milestone

Comments

@mattdowle
Copy link
Member

@mattdowle mattdowle commented Dec 13, 2018

Hi @renkun-ken.
Could you run latest data.table dev through your systems please.
There are some significant internal code changes that it would be good to run past your checks early.
Please check that more cores are being used more of the time, and whether your test suite total time is any faster or not.
Thanks!!
Matt

@mattdowle mattdowle added this to the 1.12.0 milestone Dec 13, 2018
@renkun-ken
Copy link
Member

@renkun-ken renkun-ken commented Dec 13, 2018

I notice that significant changes are made recently so I keep up with those on a daily basis. So far the system runs perfectly with 4c9a1f0.

As for the total time, it is unfortunate that fork parallelism is heavily used so that multi-threading falls to single thread and does not recover when fork is ended. Therefore I haven't observed significant difference in computing time so far.

I'll test against the latest commit very soon.

@mattdowle
Copy link
Member Author

@mattdowle mattdowle commented Dec 13, 2018

Thanks @renkun-ken!
We could maybe have a look at fork again. See past problems here :
https://github.com/Rdatatable/data.table/blob/master/src/openmp-utils.c#L73
Are you on Linux/Mac/WIndows? ( I didn't think Windows had fork, but somebody told me recently that it does.)
As those comments say, we used to have an after_fork callback that restored omp to using more than 1 thread. But the problems were on Mac and/or Intel's iomp. It may be possible to try it again just on Linux, if you're on Linux.
Does setDTthreads(0) restore multi-threaded after a parallel fork for you? If so, there's a good chance the after_fork could turn it on automatically if we put it back.

@renkun-ken
Copy link
Member

@renkun-ken renkun-ken commented Dec 13, 2018

I'm on Linux.

I test a small part of system against 0daefad and the computing time reduced 30%. I'll do a more thorough test soon.

I filed #2885 on auto-restore multi-threading after fork. fst adopts this behavior for quite a while and it works for me perfectly so far.

@renkun-ken
Copy link
Member

@renkun-ken renkun-ken commented Dec 13, 2018

Latest commit 0daefad is broken here. I'll take a closer look and file an issue about it soon.

@renkun-ken
Copy link
Member

@renkun-ken renkun-ken commented Dec 13, 2018

The problem lies on an assumed behavior that dt[TRUE] always creates a shallow copy of dt so that adding columns to the shallow copy does not influence the original data.table. See #3214.

@mattdowle
Copy link
Member Author

@mattdowle mattdowle commented Dec 14, 2018

Thanks @renkun-ken. #3214 now fixed by PR #3213 and merged.

@renkun-ken
Copy link
Member

@renkun-ken renkun-ken commented Dec 14, 2018

I filed another issue #3216 with the latest commit 5b16cc5.

@mattdowle
Copy link
Member Author

@mattdowle mattdowle commented Dec 15, 2018

@renkun-ken Many thanks for this invaluable testing!!
#3216 fixed, and #2885 (restore multi threading after fork) done too.
Please rerun whenever convenient.

@renkun-ken
Copy link
Member

@renkun-ken renkun-ken commented Dec 15, 2018

A full run with 76d4a62 is in progress...

@renkun-ken
Copy link
Member

@renkun-ken renkun-ken commented Dec 15, 2018

Just found another issue which seems to be breaking. The updated subsetting tends to under-allocating. See #3228.

@renkun-ken
Copy link
Member

@renkun-ken renkun-ken commented Dec 17, 2018

Empty .SDcols now produces an error (which I think is by design in recent changes?), following code no longer works:

dt <- data.table(x = rnorm(10), y = rnorm(10))
cols <- intersect(c("p", "q"), names(dt))
dt[, x := rowSums(.SD), .SDcols = cols]

As #3229 has just been merged, no other issues are found so far.

@mattdowle
Copy link
Member Author

@mattdowle mattdowle commented Dec 17, 2018

Thanks again for the quick turnaround!

Looks like that failed in 1.11.8 too but with a better message. Will make the error message same as before but still an error, right?

> dt <- data.table(x = rnorm(10), y = rnorm(10))
> cols <- intersect(c("p", "q"), names(dt))

v1.11.8
======
> dt[, x := rowSums(.SD), .SDcols = cols]
Error in `[.data.table`(dt, , `:=`(x, rowSums(.SD)), .SDcols = cols) : 
  RHS of assignment to existing column 'x' is zero length but not NULL. If you intend to
delete the column use NULL. Otherwise, the RHS must have length > 0; e.g., NA_integer_.
If you are trying to change the column type to be an empty list column then, as with all
column type changes, provide a full length RHS vector such as vector('list',nrow(DT));
i.e., 'plonk' in the new column.

dev error not intended
======
> dt[, x := rowSums(.SD), .SDcols = cols]
Error in `[.data.table`(dt, , `:=`(x, rowSums(.SD)), .SDcols = cols) : 
  Empty .SDcols is not yet supported.

@renkun-ken
Copy link
Member

@renkun-ken renkun-ken commented Dec 17, 2018

Sorry, I just noticed that there might be a bug in 1.11.8 on empty .SDcols that I happended to take advantage of unintentionally.

The following code works with 1.11.8:

library(data.table)

dt <- data.table(x = rnorm(10), y = rnorm(10))
gs <- list(a = c("x", "y", "z"), b = c("p", "q"))

for (g in names(gs)) {
  dt[, (g) := rowSums(.SD), .SDcols = intersect(colnames(dt), gs[[g]])]
}

But if the for-loop is run again, the RHS ... error occurs.

I think I should change my code here.

@mattdowle
Copy link
Member Author

@mattdowle mattdowle commented Dec 20, 2018

@renkun-ken With that PR just merged, it should be back to 1.11.8 behavior w.r.t. empty .SDcols. Please rerun and hopefully this resolves everything? Huge thanks again.

@renkun-ken
Copy link
Member

@renkun-ken renkun-ken commented Dec 24, 2018

Another issue: #3245.

@mattdowle
Copy link
Member Author

@mattdowle mattdowle commented Jan 4, 2019

@renkun-ken Thanks again, fixed. Please rerun.

@renkun-ken
Copy link
Member

@renkun-ken renkun-ken commented Jan 6, 2019

A full run with f73e7b0 works perfectly.

@mattdowle mattdowle closed this in 9f9f2ea Jan 11, 2019
@renkun-ken
Copy link
Member

@renkun-ken renkun-ken commented Jan 11, 2019

Looks like all issues for milestone 1.12.0 are closed. It's time to initiate a full run.

@mattdowle
Copy link
Member Author

@mattdowle mattdowle commented Jan 11, 2019

  • CRAN_Release.cmd (e.g. UBSAN, ASAN, rchk)
  • GitLab Pipeline (e.g. R 3.1.0, --enable-strict-barrier)
  • win-builder 3*OK
  • final rerun latest revdeps refreshed (748)
  • Kun Ren final go-first completed successfully

It usually takes at least a day or so for CRAN's revdep checks to run plus there's almost-always something unexpected at that stage to iron out and resubmit anyway. So I'll submit to CRAN now so that process can start. If Kun finds anything in his final rerun I can ask CRAN to halt depending on what it is.

  • submitted to CRAN
  • CRAN pre-test success email received
  • CRAN revdep status impact results email received
  • Matt replied to confirm and explain differences to submission note.
  • Accepted on CRAN

@mattdowle mattdowle reopened this Jan 11, 2019
@renkun-ken
Copy link
Member

@renkun-ken renkun-ken commented Jan 12, 2019

@mattdowle Everything works perfectly.

@jangorecki
Copy link
Member

@jangorecki jangorecki commented Jan 12, 2019

web browsers often cache website so might not show new version on cran, the following can be helpful:

Rscript -e 'data.table::fread("https://cran.r-project.org/web/packages/data.table/index.html", sep=NULL, header=FALSE)[24]'

@mattdowle
Copy link
Member Author

@mattdowle mattdowle commented Jan 12, 2019

Or Ctrl+F5 does a hard refresh.
I'll close this issue and milestone when I receive the final CRAN email.
CRAN incoming status:
https://cransays.itsalocke.com/articles/dashboard.html

@mattdowle mattdowle changed the title Requesting a go-first run please Release checks Jan 12, 2019
@mattdowle
Copy link
Member Author

@mattdowle mattdowle commented Jan 12, 2019

In some ways I think the most significant item of this release is note 1. The bug report to r-core is open with no progress. It's potentially really serious and could have affected a lot of packages for a long time, silently.
Like I did with OpenMP for Mac (which worked) the new check and long message is intended to draw attention to it. It asks folk affected to add a comment to the R-core bug report.
If there's any movement on that, I'll be delighted.
We need to gather more very precise and reproducible evidence in the day or so after it hits CRAN and before the Windows binary is made available on CRAN. I believe the most severe problem is only in that short window so we need to gather good evidence right then and there to move the needle in the minds of R-core.
If we can submit a patch then that would help even more. I had a look myself and it looked non-trivial. It likely needs the help of R-core members to explain the logic and comments. And of course testing the patch is non-trivial, time consuming and platform dependent.

Sadly, the helpful long new message won't trigger in 1.12.0. Instead there'll be a terse message that CdllVersion can't be found. That will be improved by #3282 in 1.12.2. At least silent mismatch state wont be possible which is something. For data.table anyway that has this check.

@jangorecki
Copy link
Member

@jangorecki jangorecki commented Jan 13, 2019

Be sure to tweet after cran release will be rolled out so we can get feedback from windows users on that. @mattdowle

Done: https://twitter.com/MattDowle/status/1084528873549705217

@jangorecki
Copy link
Member

@jangorecki jangorecki commented Jan 13, 2019

1.12.0 is on CRAN.
Windows users please try to install from CRAN having 1.11.8 data.table attached in another R session to verify the mechanism mentioned above to prevent dll version mismatch. As described in #3282 you might see CdllVersion not found type error. You need Rtools installed so R will build Windows binaries from source. Also see @mattdowle's comment above for details.

@jangorecki jangorecki closed this Jan 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants