Skip to content

Commit

Permalink
[jvm-packages] do not use multiple jobs to make checkpoints (#5082)
Browse files Browse the repository at this point in the history 
* temp

* temp

* tep

* address the comments

* fix stylistic issues

* fix

* external checkpoint
  • Loading branch information
@CodingCat
CodingCat authored Feb 2, 2020
1 parent fa26313 commit d7b45fb
Show file tree
Hide file tree
Showing 14 changed files with 463 additions and 319 deletions.
1 change: 1 addition & 0 deletions jvm-packages/pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
<spark.version>2.4.3</spark.version>
<scala.version>2.12.8</scala.version>
<scala.binary.version>2.12</scala.binary.version>
<hadoop.version>2.7.3</hadoop.version>
</properties>
<repositories>
<repository>
Expand Down
164 changes: 0 additions & 164 deletions ...ages/xgboost4j-spark/src/main/scala/ml/dmlc/xgboost4j/scala/spark/CheckpointManager.scala
View file

This file was deleted.

Loading

9 comments on commit d7b45fb

@CodingCat
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RAMitchell @trivialfis @hcho3 could this be in 1.0?

@hcho3
Copy link
Collaborator

@hcho3 hcho3 commented on d7b45fb Feb 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CodingCat I’m not an expert in JVM packages, so I’ll have to defer to you on that. One suggestion is to ask whether the benefit outweighs risks (e.g. risk of breaking things or worsening performance).

@CodingCat
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CodingCat I’m not an expert in JVM packages, so I’ll have to defer to you on that. One suggestion is to ask whether the benefit outweighs risks (e.g. risk of breaking things).

I tested this feature which is originally in 1.0 roadmap (sorry, I am too busy to upstream these things in a timely manner), everything looks fine

since 1.0 branch has been cut, I would get an agreement on backporting

@hcho3
Copy link
Collaborator

@hcho3 hcho3 commented on d7b45fb Feb 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also another option is to wait for 6 weeks to do 1.1 release. There are a few interesting PRs that are being left out of 1.0, and they would make a nice minor release.

@hcho3
Copy link
Collaborator

@hcho3 hcho3 commented on d7b45fb Feb 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, if it’s tested in your system, it should be okay to cherrypick this commit to 1.0 branch. Just give me your word and I’ll do it.

@hcho3
Copy link
Collaborator

@hcho3 hcho3 commented on d7b45fb Feb 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since 1.0 branch has been cut, I would get an agreement on backporting

Did not see this sentence. Let’s wait to hear back from at least one another committer.

@trams I’d like to hear from you whether this commit should be included in 1.0 release.

@RAMitchell
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine to add it if you are comfortable, scope is limited to JVM package.

@CodingCat
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, folks, I pushed to 1.0.0 branch

@trams
Copy link
Contributor

@trams trams commented on d7b45fb Feb 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to include it

Please sign in to comment.