Experimental support for HDFS #1243

ebernhardson · 2018-02-15T19:23:40Z

Read and write datsets from hdfs.
Only enabled when cmake is run with -DUSE_HDFS:BOOL=TRUE
Introduces VirtualFile(Reader|Writer) to asbtract VFS differences

msftclas · 2018-02-15T19:23:51Z

All CLA requirements met.

ebernhardson · 2018-02-15T19:26:49Z

I've forwarded the CLA to legal, typically resolved in a day.

guolinke · 2018-02-16T01:01:50Z

include/LightGBM/bin.h

@@ -98,7 +98,7 @@ class BinMapper {
  * \brief Save binary data to file
  * \param file File want to write
  */
-  void SaveBinaryToFile(FILE* file) const;
+  void SaveBinaryToFile(std::shared_ptr<VirtualFileWriter> file) const;


why use share_ptr ? we often use const raw pointers. what are the advantages of share_ptr ?

It's only a personal preference, these objects are very low volume so the overhead should be minimal, and the shared_ptr made thinking about the lifetime much easier. If you prefer though i can re-work this so the base instance gets allocated on the stack and we pass const pointers. Stack allocation should keep the relatively simple lifetime.

okay, got it. The pointer is actual not 'const' inside these functions.

ebernhardson · 2018-02-16T01:06:46Z

include/LightGBM/utils/file_io.h

+      if (fs_ == NULL) {
+        auto namenode = std::vector<char>(100);
+        if (1 != sscanf(filename_.c_str(), "hdfs://%99[^/]/", namenode.data())) {
+          Log::Warning("Could not parse namenode from uri [%s]", filename_.c_str());


Log::Fatal, or at least return false

ebernhardson · 2018-02-16T01:07:17Z

include/LightGBM/utils/file_io.h

+  }
+
+  static inline bool Exists(const char* filename) {
+    FILE* file_ = fopen(filename, "rb");


missing ifdef's for MSC

ebernhardson · 2018-02-16T01:08:29Z

include/LightGBM/utils/file_io.h

+  }
+
+  virtual bool Init() {
+    if (file_ == NULL) {


this init code is pretty much duplicated for Read/Write. Can it be shared?

ebernhardson · 2018-02-16T01:22:28Z

include/LightGBM/bin.h

@@ -98,7 +98,7 @@ class BinMapper {
  * \brief Save binary data to file
  * \param file File want to write
  */
-  void SaveBinaryToFile(FILE* file) const;
+  void SaveBinaryToFile(std::shared_ptr<VirtualFileWriter> file) const;


It's only a personal preference, these objects are very low volume so the overhead should be minimal, and the shared_ptr made thinking about the lifetime much easier. If you prefer though i can re-work this so the base instance gets allocated on the stack and we pass const pointers. Stack allocation should keep the relatively simple lifetime.

guolinke · 2018-02-17T11:13:13Z

include/LightGBM/utils/file_io.h

+
+  virtual bool Init()=0;
+  virtual size_t Write(const void* data, size_t bytes) const=0;
+  static std::shared_ptr<VirtualFileWriter> Make(const char* filename);


unique_ptr ?

guolinke · 2018-02-17T11:17:22Z

src/io/parser.cpp

  }
  std::string line1, line2;
+  size_t buffer_size = 32*1024;


I think this may is not enough for the long lines.

I've updated this so it should work for any length line possible, and added a test on the python package to assert it's working.

guolinke · 2018-02-17T11:22:38Z

@ebernhardson sorry, we are in the Chinese New Year holiday, so the review is slow.
Did you test this ?

@StrikerRUS it seems the check-the-docs test failed again.

StrikerRUS · 2018-02-17T11:47:30Z

@guolinke Yeah, I see...

Sites with Yahoo dataset and MPICH paper are down. SWIG site returns this warning: Project web is currently offline pending the final migration of its data to our new datacenter.

I think that Yahoo and SWIG will be online in a few days and we should just wait. Not sure about site with paper, maybe it should be replaced with another site.

StrikerRUS · 2018-02-20T10:16:22Z

@guolinke All sites are alive now, I've re-run Travis.

* Read and write datsets from hdfs. * Only enabled when cmake is run with -DUSE_HDFS:BOOL=TRUE * Introduces VirtualFile(Reader|Writer) to asbtract VFS differences

ebernhardson · 2018-02-20T18:28:44Z

@guolinke I've tested this against a hadoop cluster I have available, running CDH 5.5, on input files up to 100GB. Works for reading in text or binary datasets and writing out binary datasets. I haven't explicitly tested the other code paths like predict but see no reason it should vary as long as the local file tests also pass.

I've also added a second patch to this PR, but i could split it into a second if preferred. The second patch allows providing a comma separated list of filenames and have them treated a single file concatenated together. This is particularly important for hdfs as it is very typical (and highly performant when writing) to write out a single large file as lots of smaller files of perhaps 256M or 1GB each. Some python tests were added to show this working.

guolinke · 2018-02-23T06:02:13Z

@ebernhardson thanks very much.
I think use another PR for the mulit-file support is better. And we can merge this PR first.

guolinke · 2018-02-25T02:04:20Z

@ebernhardson
Do you have any speedtest for the IO time (dataset loading) after this PR ?
you can use higgs(https://github.com/guolinke/boosting_tree_benchmarks/tree/master/data) dataset to have a quick test.

ebernhardson · 2018-02-26T18:31:35Z

Ran each of these for 10 iterations against the full higgs dataset (xeon [email protected]). Numbers are all close I would judge the patch as having no performance impact. HDFS speed is also not so bad at 80% of line speed (and the nic is shared).

	min	mean	max
master	11.1s	11.7s	14.7s
hdfs patch, USE_HDFS=0, local file	10.9s	11.4s	13.9s
hdfs patch, USE_HDFS=1, local file	11.2s	11.9s	14.8s
hdfs patch, hdfs file	1m15s	1m18s	1m22s

guolinke · 2018-02-27T04:52:09Z

@ebernhardson thanks very much. It is a very great PR.

guolinke · 2018-02-27T05:03:37Z

src/io/file_io.cpp

+  LocalFile(const std::string& filename, const std::string& mode) : filename_(filename), mode_(mode) {}
+  virtual ~LocalFile() {
+    if (file_ != NULL) {
+      fclose(file_);


set file to NULL after close ? I am not sure will fclose change FILE to NULL or not.

* minor change * unix line break

ebernhardson force-pushed the hdfs branch from 205a770 to a55bf04 Compare February 15, 2018 19:31

guolinke reviewed Feb 16, 2018

View reviewed changes

ebernhardson commented Feb 16, 2018

View reviewed changes

ebernhardson force-pushed the hdfs branch from a55bf04 to 3d7fdd3 Compare February 17, 2018 05:07

guolinke reviewed Feb 17, 2018

View reviewed changes

ebernhardson force-pushed the hdfs branch from 3d7fdd3 to b9133af Compare February 20, 2018 17:45

Experimental support for HDFS

2a2ac0d

* Read and write datsets from hdfs. * Only enabled when cmake is run with -DUSE_HDFS:BOOL=TRUE * Introduces VirtualFile(Reader|Writer) to asbtract VFS differences

ebernhardson force-pushed the hdfs branch from b9133af to a761ed0 Compare February 20, 2018 18:23

ebernhardson force-pushed the hdfs branch from a761ed0 to 2a2ac0d Compare February 23, 2018 16:34

guolinke approved these changes Feb 25, 2018

View reviewed changes

guolinke merged commit 7e186a5 into microsoft:master Feb 27, 2018

guolinke reviewed Feb 27, 2018

View reviewed changes

chivee mentioned this pull request Feb 27, 2018

minor change for #1243 #1250

Merged

guolinke pushed a commit that referenced this pull request Feb 27, 2018

minor change for #1243 (#1250)

2848176

* minor change * unix line break

StrikerRUS mentioned this pull request Feb 27, 2018

[python][docs] added info about and possibility to install Python-package HDFS version #1252

Merged

chivee mentioned this pull request Mar 16, 2018

how to use lightgbm of hdfs version? #1269

Closed

lock bot locked as resolved and limited conversation to collaborators Mar 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experimental support for HDFS #1243

Experimental support for HDFS #1243

ebernhardson commented Feb 15, 2018

msftclas commented Feb 15, 2018 •

edited

Loading

ebernhardson commented Feb 15, 2018 •

edited

Loading

guolinke Feb 16, 2018

ebernhardson Feb 16, 2018

guolinke Feb 16, 2018

ebernhardson Feb 16, 2018

ebernhardson Feb 16, 2018

ebernhardson Feb 16, 2018

ebernhardson Feb 16, 2018

guolinke Feb 17, 2018

guolinke Feb 17, 2018

ebernhardson Feb 20, 2018

guolinke commented Feb 17, 2018

StrikerRUS commented Feb 17, 2018

StrikerRUS commented Feb 20, 2018

ebernhardson commented Feb 20, 2018

guolinke commented Feb 23, 2018 •

edited

Loading

guolinke commented Feb 25, 2018

ebernhardson commented Feb 26, 2018 •

edited

Loading

guolinke commented Feb 27, 2018

guolinke Feb 27, 2018

Experimental support for HDFS #1243

Experimental support for HDFS #1243

Conversation

ebernhardson commented Feb 15, 2018

msftclas commented Feb 15, 2018 • edited Loading

ebernhardson commented Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guolinke commented Feb 17, 2018

StrikerRUS commented Feb 17, 2018

StrikerRUS commented Feb 20, 2018

ebernhardson commented Feb 20, 2018

guolinke commented Feb 23, 2018 • edited Loading

guolinke commented Feb 25, 2018

ebernhardson commented Feb 26, 2018 • edited Loading

guolinke commented Feb 27, 2018

Choose a reason for hiding this comment

msftclas commented Feb 15, 2018 •

edited

Loading

ebernhardson commented Feb 15, 2018 •

edited

Loading

guolinke commented Feb 23, 2018 •

edited

Loading

ebernhardson commented Feb 26, 2018 •

edited

Loading