PUBDEV-4639: Added requirement for data.table. #4265

wendycwong · 2020-01-28T02:36:07Z

This PR enable data.table by default as specified by JIRA: https://0xdata.atlassian.net/browse/PUBDEV-4639.

honzasterba

Should probably change the default value fo use arg

wendycwong · 2020-01-28T18:21:46Z

Thanks for the review. I don't understand what that means. Please help? W

wendycwong · 2020-01-29T17:58:44Z

Honza: I have added the change in h2o-3-DESCRIPTION.template. Let me know if that is the right place. If not, please point me to the right file.

honzasterba · 2020-01-30T03:39:37Z

@wendycwong no, I think we want to keep data.table in suggest since its not required, the data.table support should only enable itself when data.table is installed

wendycwong · 2020-01-30T21:40:44Z

@honzasterba Can you tell me exactly what I need to change then. Thanks. W

honzasterba · 2020-01-31T11:03:24Z

I change to DESCRIPTION should be reverted as I see you did.
Also I added a commit to this PR but it is gone so I added the change again.

wendycwong · 2020-01-31T17:49:41Z

Honza: Thanks for the help. Do you think this PR is ready to go? Or more changes are needed? I have been testing it continuously and it seems to work fine. Let me know. Thanks, Wendy

honzasterba

if the tests are green I think its fine

honzasterba · 2020-02-01T02:22:23Z

maybe check if there is some docs that need updating @angela0xdata

wendycwong · 2020-02-03T20:40:13Z

@honzasterba: There are two runits that are failing in RSmoke test. That needs to be resolved before this PR can merge.

michalkurka · 2020-02-13T15:48:28Z

Needs fixing before merging:

[2020-02-13T01:04:38.981Z] * checking R code for possible problems

[2020-02-13T01:04:38.981Z] use.package: no visible global function definition for

[2020-02-13T01:04:38.981Z]   ‘installed.packages’

[2020-02-13T01:04:38.981Z] Undefined global functions or variables:

[2020-02-13T01:04:38.981Z]   installed.packages

[2020-02-13T01:04:38.981Z] Consider adding

[2020-02-13T01:04:38.981Z]   importFrom("utils", "installed.packages")

AFAIK @ledell offered to help with that

honzasterba · 2020-02-17T07:44:40Z

I have pushed a fix for the CRAN issue and also the actual change of the default value for data.table use.
@mattdowle could you please take a look at the test failures?

mattdowle · 2020-02-21T09:03:15Z

@honzasterba The PR I submitted #4308 does not appear to have been merged yet @wendycwong @ledell ?

honzasterba · 2020-02-21T09:10:43Z

@mattdowle I believe wendy cherry picked your commit into this PR, I am now working on fixing the rest of the tests

mattdowle · 2020-02-21T09:57:43Z

@honzasterba Ah ok I see the cherry pick now.
I was looking at the error in example(as.h2o) and I see your recent commit. But I see fail again just now :

Error in .subset(x, j) : invalid subscript type 'list'
[2020-02-21T09:50:30.146Z] Calls: as.h2o ... .h2o.calc_stm_svm -> [ -> [.data.table -> [.data.frame
[2020-02-21T09:50:30.146Z] Execution halted

Did you write earlier above in this PR that data.table is downgraded to suggests in DESCRIPTION? If so that will affect using data.table syntax. It seems that as.h2o.Matrix is using data.table syntax. For that to work data.table needs to know h2o is data.table-aware.
https://stackoverflow.com/a/10529888

There is a flag that can be set in h2o's NAMESPACE file (.datatable.aware) that would allow h2o to use data.table syntax while still only suggesting it. We haven't had the situation before that a package (h2o in this case) wants to use data.table syntax itself, but only optionally depending on that package's user.

Options that spring to mind are :

Remove the use of data.table syntax within h2o package itself. Just use data.table for read and write.
h2o import data.table. But that would require users to install data.table which I understand you'd prefer not to.
Set .datatable.aware.

However, with both options 2 and 3, if a user passes a data.table to a function in h2o that is really expecting a data.frame (i.e. it is using data.frame syntax), then unexpected behaviour could occur. When a package states it is data.table-aware, it really has to be fully aware throughout all exported functions. Packages which accept data.frame from a user, but want to use data.table syntax, should have an as.data.table() call at the start of each user-visible function that use data.table syntax, and an as.data.frame() call at the start of each exported function that uses data.frame syntax.

I think option 1 is by far the safest option in the case of h2o package.

honzasterba · 2020-02-21T10:11:49Z

yes, I have fixed the data-table aware issue yesterday, but it seems that @wendycwong did another rebase afterwards and my commit is lost :(

wendycwong · 2020-02-21T17:04:37Z

Sorry guys. I will take my hands off this PR now.

mattdowle · 2020-02-22T09:23:15Z

This branch passes R CMD check locally for me. When running on Jenkins it seems to be running example(as.h2o) even though that code is inside a \dontrun{ ... } section. Has anyone seen that before?
Anyway, I guess something is testing donotrun blocks then, I managed to reproduce locally by running that donotrun manually...

h2o-core/src/main/java/water/api/DatasetServlet.java

honzasterba · 2020-02-24T15:22:42Z

@mattdowle @ledell please review, I have decided to use .datatable.aware since the only place we should get in touch with data.frames is as.h2o, any other concerns?

michalkurka · 2020-02-27T17:42:18Z

h2o-r/tests/testdir_jira/runit_pubdev_2844.R

@@ -5,7 +5,9 @@ test.pubdev_2844 <- function() {

  df1 <- iris
  h2o.no_progress()
-
+  # enable using of data.table for as.data.frame
+  options(h2o.fread=TRUE)


does this also enable it for the rest of the test suite? or is it just this test?

I think it probably does, will fix

michalkurka · 2020-02-27T17:47:04Z

@honzasterba looks good to me, but please change the target branch to master and lets target 3.30 for this feature

also try to fix as.data.frame

…ly reverting; unrelated to data.table per se

… minimally reverting; unrelated to data.table per se" This reverts commit 668b7dc.

mattdowle · 2020-02-27T22:36:22Z

h2o-r/h2o-package/R/frame.R

@@ -3530,6 +3530,8 @@ as.data.frame.H2OFrame <- function(x, ...) {
  # Versions of R prior to 3.1 should not use hex string.
  # Versions of R including 3.1 and later should use hex string.
  useHexString <- getRversion() >= "3.1"
+  # We cannot use data.table by default since its handling of escaping inside quoted csv values is not very good
+  # in some edge cases its simply impossible to load data in correct format without additional post processing


What specifically do you mean by "not very good"? This is unhelpful to write in this way. Tell me what's wrong and I'll fix it. Probably already known somewhere and I'll raise the priority. We're on the same team here.

Sorry for being so ambiguous. On second thought I should have been more to the point and link to existing issue.
This is the issue I was reffering to:
This is the issue: Rdatatable/data.table#1109
Here is the test that I was not able to make pass with data.table even with changes on the back-end: https://github.com/h2oai/h2o-3/pull/4265/files#diff-c87a495277fa01b186f5300c48973bf6R33

wendycwong requested a review from ledell January 28, 2020 02:36

honzasterba reviewed Jan 28, 2020

View reviewed changes

wendycwong force-pushed the pubdev-4639-use-data-table-rel-yu branch from 2f46e16 to fe16bb5 Compare January 29, 2020 17:57

wendycwong force-pushed the pubdev-4639-use-data-table-rel-yu branch from fe16bb5 to 7b72d37 Compare January 30, 2020 21:43

honzasterba approved these changes Feb 1, 2020

View reviewed changes

wendycwong added the do not merge For PRs that are not supposed to be merged label Feb 3, 2020

wendycwong force-pushed the pubdev-4639-use-data-table-rel-yu branch 3 times, most recently from bfa0de1 to 5a4c85e Compare February 12, 2020 17:48

wendycwong force-pushed the pubdev-4639-use-data-table-rel-yu branch from 3ef138a to ec3c3e8 Compare February 14, 2020 17:08

wendycwong force-pushed the pubdev-4639-use-data-table-rel-yu branch from 333e7d3 to 7d17920 Compare February 19, 2020 19:00

honzasterba requested a review from mattdowle February 20, 2020 15:35

wendycwong force-pushed the pubdev-4639-use-data-table-rel-yu branch from fef748f to 2fa26f1 Compare February 20, 2020 17:19

honzasterba self-requested a review February 21, 2020 10:20

honzasterba removed their request for review February 24, 2020 08:55

honzasterba self-assigned this Feb 24, 2020

Pscheidl reviewed Feb 24, 2020

View reviewed changes

h2o-core/src/main/java/water/api/DatasetServlet.java Show resolved Hide resolved

Pscheidl approved these changes Feb 24, 2020

View reviewed changes

honzasterba requested a review from michalkurka February 27, 2020 05:41

michalkurka reviewed Feb 27, 2020

View reviewed changes

honzasterba added please review and removed do not merge For PRs that are not supposed to be merged labels Feb 27, 2020

wendycwong and others added 13 commits February 27, 2020 18:57

PUBDEV-4639: use data.table if possible.

c37d392

workaround for fread length-1 colClasses=NA_character_ bug #4237

fdb4434

actually change the default value

48c6dc5

fix R CRAN check

1436a66

fix as.h2o.Matrix

0040575

bring back lost change

7484cd5

also try to fix as.data.frame

debugged and fixed changes in this PR to .h2o.calc_stm_svm by minimal…

22f0f1c

…ly reverting; unrelated to data.table per se

Revert "debugged and fixed changes in this PR to .h2o.calc_stm_svm by…

5d8a723

… minimally reverting; unrelated to data.table per se" This reverts commit 668b7dc.

fix matrix to svm serialization

7b91d9d

more fixes for data.table

d84c8c4

comment data.table limitations

0c72748

fix as.h2o test

3dd9456

fix test

559fafa

honzasterba changed the base branch from rel-yu to master February 27, 2020 17:58

honzasterba force-pushed the pubdev-4639-use-data-table-rel-yu branch from 973f02f to 559fafa Compare February 27, 2020 17:58

make test isolated

f144dbc

honzasterba merged commit 3a799f0 into master Feb 27, 2020

honzasterba deleted the pubdev-4639-use-data-table-rel-yu branch February 27, 2020 20:14

mattdowle reviewed Feb 27, 2020

View reviewed changes

hasithjp mentioned this pull request May 15, 2023

Turn on data.table support in h2o R package by default #11521

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PUBDEV-4639: Added requirement for data.table. #4265

PUBDEV-4639: Added requirement for data.table. #4265

wendycwong commented Jan 28, 2020

honzasterba left a comment

wendycwong commented Jan 28, 2020

wendycwong commented Jan 29, 2020

honzasterba commented Jan 30, 2020

wendycwong commented Jan 30, 2020

honzasterba commented Jan 31, 2020

wendycwong commented Jan 31, 2020

honzasterba left a comment

honzasterba commented Feb 1, 2020

wendycwong commented Feb 3, 2020 •

edited

Loading

michalkurka commented Feb 13, 2020

honzasterba commented Feb 17, 2020

mattdowle commented Feb 21, 2020

honzasterba commented Feb 21, 2020

mattdowle commented Feb 21, 2020 •

edited

Loading

honzasterba commented Feb 21, 2020

wendycwong commented Feb 21, 2020

mattdowle commented Feb 22, 2020 •

edited

Loading

honzasterba commented Feb 24, 2020

michalkurka Feb 27, 2020

honzasterba Feb 27, 2020

michalkurka commented Feb 27, 2020

mattdowle Feb 27, 2020

honzasterba Feb 28, 2020

PUBDEV-4639: Added requirement for data.table. #4265

PUBDEV-4639: Added requirement for data.table. #4265

Conversation

wendycwong commented Jan 28, 2020

honzasterba left a comment

Choose a reason for hiding this comment

wendycwong commented Jan 28, 2020

wendycwong commented Jan 29, 2020

honzasterba commented Jan 30, 2020

wendycwong commented Jan 30, 2020

honzasterba commented Jan 31, 2020

wendycwong commented Jan 31, 2020

honzasterba left a comment

Choose a reason for hiding this comment

honzasterba commented Feb 1, 2020

wendycwong commented Feb 3, 2020 • edited Loading

michalkurka commented Feb 13, 2020

honzasterba commented Feb 17, 2020

mattdowle commented Feb 21, 2020

honzasterba commented Feb 21, 2020

mattdowle commented Feb 21, 2020 • edited Loading

honzasterba commented Feb 21, 2020

wendycwong commented Feb 21, 2020

mattdowle commented Feb 22, 2020 • edited Loading

honzasterba commented Feb 24, 2020

michalkurka Feb 27, 2020

Choose a reason for hiding this comment

honzasterba Feb 27, 2020

Choose a reason for hiding this comment

michalkurka commented Feb 27, 2020

mattdowle Feb 27, 2020

Choose a reason for hiding this comment

honzasterba Feb 28, 2020

Choose a reason for hiding this comment

wendycwong commented Feb 3, 2020 •

edited

Loading

mattdowle commented Feb 21, 2020 •

edited

Loading

mattdowle commented Feb 22, 2020 •

edited

Loading