fix: LightGBMRanker should repartition by grouping column #778
Conversation
|
💖 Thanks for opening your first pull request! 💖 We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should make sure your first commit and PR title start with a semantic prefix. This helps us to create release messages and credit you for your hard work!
Make sure to check out the developer guide for guidance on testing your change. |
JoanFM
changed the title
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Codecov Report
@@ Coverage Diff @@
## master #778 +/- ##
==========================================
+ Coverage 41.41% 49.44% +8.02%
==========================================
Files 241 185 -56
Lines 9701 8515 -1186
Branches 525 518 -7
==========================================
+ Hits 4018 4210 +192
+ Misses 5683 4305 -1378
Continue to review full report at Codecov.
|
src/main/scala/com/microsoft/ml/spark/lightgbm/LightGBMRanker.scala
Outdated
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Can I get some help with the tests? It is expected for this PR to cause diffs but with the class structure in the tests I am having real trouble seeing what I need to check and update, can someone give me a hand on this? Thank you very much |
|
@JoanFM it looks like some lightgbm ranker tests are failing, not sure why yet. Will take a look when I get a chance. |
|
Yes it is expected I think, I observed that model result is depending on partitioning of the data, but I cannot track down where the expected output is taken from or anything. The complete fuzzying class structure is a little too complex if u are not familiar with it I guess |
Could you point me in some direction to help me fix it on my own if you do not have the time? |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@JoanFM yes, will take a look sorry. Not sure where to point you to otherwise, as I'm not sure why it is failing myself. I was just going to get your branch and run the tests locally to see if I could reproduce the test failures. |
|
@imatiach-msft I could reproduce the issue locally, I think it is natural to have differences because now every lightGBM instance see a different set of samples. |
…eterministic, we cannot guarantee same model after fit with same parameters and fitDF
|
Hello @imatiach-msft , I have seen the source of discrepancy. At some point we test that when Ranker is saved, which I understand that is just the configuration, and the same loaded result in the same prediction. But since repartition is non-deterministic I think, we cannot guarantee that we will get the same LightGBM model with the same trees and gains. Therefore, there are some discrepancies. I think this test may be meaningless in this case. Let me know if it is okey this last change |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Any idea what is failing? is there some test that is failing due to this change? Is there a way to see the cause of the failure? |
|
@JoanFM yes it looks like one of the notebook tests timed out: I see this error when I click "details" on the build. I'm guessing you don't have access because the devops account is still not public. I mentioned to @mhamilton723 that the devops account needs to be made public so external developers can view it, but he hasn't had the cycles to work on this. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
it looks like datasets failed to download which caused all of the lightgbm tests to fail. Weird. Hoping it's a random issue, I think it might be related to the change @eisber recently did to update the datasets zip file. |
|
hmm it looks like that was some random networking failure with downloading the files @eisber , the test seems to be passing now. Maybe we need to add some retry logic there in the future if download/unzip fails. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
) * LightGBMRanker should repartition by grouping column * Ignore estimators in VerifyLightGBMRanker. Since repartition is not deterministic, we cannot guarantee same model after fit with same parameters and fitDF * Add option to turn ON and OFF repartitioning, and setting it off for test Co-authored-by: Ilya Matiach <ilmat@microsoft.com> Co-authored-by: Mark Hamilton <mhamilton723@gmail.com>
|
@JoanFM Why force materialization is needed here ? |
|
Hey @liyzcj , I am a little disconnected from this project, but I guess it may had to do with the lazyness of the operation? Not 100% sure at this moment, may not even be needed. |
|
@JoanFM Thanks for reply, I may test for this later. |
fix: Repartition by groupingColumn to guarantee proper LightGBMRanker results
Consider repartitioning by grouping column since for now it is only guaranteed to have groups together in every partition but for correctness every group should be found together in the same partition.
A comment in #682 seems to agree on this statement