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

support dataset rows more then max(int32_t) (add CMAKE option) #5540

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

junpeng0715
Copy link

related to #5454 , add a CMAKE option to switch int32 and int64

@junpeng0715 junpeng0715 changed the title [WIP] support dataset rows more then max(int32_t) [WIP] support dataset rows more then max(int32_t) (add CMAKE option) Oct 13, 2022
@guolinke
Copy link
Collaborator

@shiyu1994 can you help to review this PR?

@junpeng0715
Copy link
Author

@guolinke @shiyu1994
Hi!
Base on #5454 , a Cmake option (USE_DATASET_INT64) has been added to control whether int32 or int64 is used
c_api parts are fixed to int64 and converted into data_size_t during internal processing.

Since the current SWIG does not seem to support int64 very well, some temporary fixes has been carried out in the "CMakeList.txt".
I don't think the current modification affects the use of int32 users.

Since we also use SynapseML, we also changed the LightGBM part of SynapseML to the Long type.
We'll submit to SynapseML later.

For the testing part, we have tested datasets over int32 in our own project (SynapseML + LGBM), and I think it can be learned normally using int64,
But instead of putting this part of the test in LGBM, just adding an int64 compilation part of the pipeline to LGBM.

We sincerely hope that you will review it.

Best Regards

@guolinke
Copy link
Collaborator

@junpeng0715 can you also add a CI test, to build with int64_t type and run some tests?

@guolinke
Copy link
Collaborator

@junpeng0715 can you also add a CI test, to build with int64_t type and run some tests?

Oh, I saw the test, thank you!

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

I'd like a chance to also review these quite impactful changes. Before I do...why was a new PR opened instead of just pushing changes to #5454? Can we now close #5454?

Also, please check you .gitconfig...it looks like many of your commits are not tied to your GitHub account. See #5532 (comment).

@junpeng0715
Copy link
Author

@microsoft-github-policy-service agree company="ACN"

@junpeng0715
Copy link
Author

I'd like a chance to also review these quite impactful changes. Before I do...why was a new PR opened instead of just pushing changes to #5454? Can we now close #5454?

@jameslamb It is a PR without Cmake Option modification as a backup. I think we can close it.

Also, please check you .gitconfig...it looks like many of your commits are not tied to your GitHub account. See #5532 (comment).

Thank you for your tips, modified it!

junpeng0715 added a commit to junpeng0715/LightGBM that referenced this pull request Oct 18, 2022
@jameslamb
Copy link
Collaborator

jameslamb commented Oct 18, 2022

Ok thanks, I closed #5554.

In the future, please just push new commits in response to reviewers' comments, instead of opening new PRs, when reviewers ask for a different approach. That keeps all of the conversation focused in one place, and ensures that other conversations threads that linked to the work you were doing don't need to now to link to multiple very-similar pull requests.

@junpeng0715 junpeng0715 requested review from jameslamb and removed request for jmoralez, shiyu1994, guolinke and StrikerRUS October 18, 2022 06:25
@junpeng0715
Copy link
Author

@guolinke @jameslamb @shiyu1994
Hi Guys
How to trigger the pipeline for R-packages?CUDA? etc. I think there were some errors.

@junpeng0715
Copy link
Author

Hi @guolinke @shiyu1994
Can you give some suggestions on how to fix CUDA version and R-package(windows-lastest) issues?
image

@junpeng0715
Copy link
Author

Could you please help review for this PR?

We will review this once it is passing CI. Please fix the failing lint task (https://github.com/microsoft/LightGBM/actions/runs/3327507460/jobs/5510896951) and Azure DevOps jobs (https://dev.azure.com/lightgbm-ci/lightgbm-ci/_build/results?buildId=13781&view=results).

CUDA builds have also been blocked in this repo for 9 days now...we are blocked waiting on #5546. Sorry for the inconvenience.

I will check lint task, I think failing DevOps jobs related to MacOS is working on #5555, CUDA issue is #5546 as you said.
There also hava some R-package failings in this PR and other PR, unrelated to my codes.

@junpeng0715 junpeng0715 changed the title [WIP] support dataset rows more then max(int32_t) (add CMAKE option) support dataset rows more then max(int32_t) (add CMAKE option) Oct 31, 2022
@junpeng0715
Copy link
Author

Hi @jameslamb
The Static Analysis issue for this PR has been resolved.

The block issues are as below, am I right?
R-package: #5563
MacOS : #5555
CUDA: #5546

@jameslamb
Copy link
Collaborator

The block issues are as below, am I right?

No. #5555 is not a blocking issue.

The following need to be fixed before any pull requests in this repo will build successfully.

Fixing #5546 requires manual action that only @shiyu1994 can do. #5549 and #5562 will be fixed by #5563.

@junpeng0715
Copy link
Author

The block issues are as below, am I right?

No. #5555 is not a blocking issue.

The following need to be fixed before any pull requests in this repo will build successfully.

Fixing #5546 requires manual action that only @shiyu1994 can do. #5549 and #5562 will be fixed by #5563.

Thank you @jameslamb ! I understand clearly now!

@jameslamb
Copy link
Collaborator

@junpeng0715 The CI issues have been fixed. Please merge latest master into this branch so we can review.

@junpeng0715
Copy link
Author

junpeng0715 commented Nov 4, 2022

@junpeng0715 The CI issues have been fixed. Please merge latest master into this branch so we can review.

@jameslamb Merged the lastest master. Got a new error in Linux inference[Initialize job] , "File not found: 'docker'"

@junpeng0715
Copy link
Author

@jameslamb
I resolved the conflicts, could you help trigger pipeline ?

@jameslamb
Copy link
Collaborator

could you help trigger pipeline

sure, re-triggered. As I mentioned previously (#5540 (comment)), you could also consider making a separate, small, non-controversial contribution (like one of the items from #5313) so that you wouldn't be considered a first-time contributor any more and would need to @ us every time you want the CI jobs to run.

@junpeng0715
Copy link
Author

Hi @jameslamb
Got R-package error, I think it is also an OpenMP issue

test-omp.c:1:10: fatal error: 'omp.h' file not found
#include <omp.h>
^~~~~~~
1 error generated.
*** OpenMP not supported! data.table uses OpenMP to automatically

@junpeng0715
Copy link
Author

hi @jameslamb and @guolinke
I will no longer be on this task, Thank you for your long term support, may continue to contribute to LightGBM through my personal account.
Could you discuss the scheduling of this PR, microsoft/SynapseML#1684 is waiting for the plan.

@sinhrks will handle the PR if further modification required. He belongs to the same company and agreed with CLA.

Best Regards

@guolinke
Copy link
Collaborator

@junpeng0715
Sorry to hear that, thank you so much for the great work!
And looking forward to your contributions in the future!

We will move this PR forward, and discuss at microsoft/SynapseML#1684

@jameslamb
Copy link
Collaborator

We will move this PR forward,

@guolinke are you still interested in pursuing this PR? It's been 4 months since the most recent commit.

@guolinke
Copy link
Collaborator

I think we can work on the 4.0.0 release first, then revisit these PRs after that. cc @shiyu1994 .

We will move this PR forward,

@guolinke are you still interested in pursuing this PR? It's been 4 months since the most recent commit.

@sinhrks
Copy link

sinhrks commented Feb 26, 2023

Thx for following up. Is it ok for me to prepare another PR to rebase this?

@guolinke
Copy link
Collaborator

Thx for following up. Is it ok for me to prepare another PR to rebase this?

Sure, please go ahead.

@PankajMerisha
Copy link

hi Team,
We are unable to train our models due to this issue. please refer: microsoft/SynapseML#2014
If there is any workaround please let me know

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants