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

Implement secure boost scheme - secure evaluation and validation (during training) without local feature leakage #10079

Merged
merged 40 commits into from
May 16, 2024

Conversation

ZiyueXu77
Copy link

@ZiyueXu77 ZiyueXu77 commented Feb 27, 2024

For implementing Vertical Federated Learning with Secure Features, as discussed in
#9987
This part is independent from the encryption and the alternative vertical pipeline. The purpose is to avoid leaking the real cut value information from participants. Hence add as a separate PR.
This PR is based on #10037, which should be reviewed and merged first.

ZiyueXu77 and others added 29 commits January 31, 2024 10:48
…lobal best split, but need to further apply split correctly
…lobal best split, but need to further apply split correctly
Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

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

The code looks good to me overall. We can merge it once we have some basic unittests.

As for integration tests in Python with nvflare (in future PRs), we can assert that

  • models are different for different workers.
  • predictions are the same
  • evaluation result are the same
  • only works if the 0th worker has the label.

I highly recommend using the hypothesis test framework (see python tests in xgboost and search the term hypothesis).

@ZiyueXu77
Copy link
Author

The code looks good to me overall. We can merge it once we have some basic unittests.

As for integration tests in Python with nvflare (in future PRs), we can assert that

  • models are different for different workers.
  • predictions are the same
  • evaluation result are the same
  • only works if the 0th worker has the label.

I highly recommend using the hypothesis test framework (see python tests in xgboost and search the term hypothesis).

Thanks! @YuanTingHsieh , could you add the unit tests according to @trivialfis 's suggestions?

@trivialfis
Copy link
Member

could you add the unit tests according to @trivialfis 's suggestions

Those points are all for integration tests not for small unittest. I think the integration tests in Python with nvflare will take more effort, we don't need to rush it in this PR.

@trivialfis
Copy link
Member

Hi, is there any update?

@ZiyueXu77
Copy link
Author

Hi, is there any update?

Thanks for asking! :) @YuanTingHsieh has been busy with a related NVFlare release in the past two weeks, now the release is close to finish, he will have time to work on this soon.

@ZiyueXu77
Copy link
Author

ZiyueXu77 commented Apr 15, 2024

@trivialfis Yuanting just added some unit tests, seems there is a failed R-test, but not sure if it is related to our modifications, the error message being

* checking package namespace information ... OK
* checking package dependencies ... ERROR
Packages suggested but not available: 'ggplot2', 'DiagrammeR', 'igraph'
.........
Traceback (most recent call last):
Ncpus: 4
  File "/__w/xgboost/xgboost/tests/ci_build/test_r_package.py", line 359, in <module>
    main(args)
  File "/__w/xgboost/xgboost/tests/ci_build/test_utils.py", line 52, in inner
    r = func(*args, **kwargs)
        ^^^^^^^^^^^^^^^^^^^^^
  File "/__w/xgboost/xgboost/tests/ci_build/test_r_package.py", line 307, in main
    check_rpackage(tarball)
  File "/__w/xgboost/xgboost/tests/ci_build/test_utils.py", line 31, in inner
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/__w/xgboost/xgboost/tests/ci_build/test_utils.py", line 52, in inner
    r = func(*args, **kwargs)
        ^^^^^^^^^^^^^^^^^^^^^
  File "/__w/xgboost/xgboost/tests/ci_build/test_r_package.py", line 166, in check_rpackage
    with open(rcheck_dir / "00install.out", "r") as fd:
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: 'xgboost.Rcheck/00install.out'

@trivialfis
Copy link
Member

That should be unrelated, will look into this PR today.

@ZiyueXu77
Copy link
Author

Hi @trivialfis , thanks for the updates, just merged it.
Everything passed, except an error for R regarding "matrix":
2024-04-29T13:40:51.6394593Z make: *** [Makefile:291: Matrix.ts] Error 1

@trivialfis
Copy link
Member

Triggered the rest of the CI.

@ZiyueXu77
Copy link
Author

Hi @trivialfis , there are 3 failed checks, but I think they align with the rebase merge, shall we just merge this? Thanks!

@trivialfis
Copy link
Member

Please fix errors on buildkite.

@ZiyueXu77
Copy link
Author

Please fix errors on buildkite.

The error is
C:\buildkite-agent\builds\buildkite-windows-cpu-autoscaling-group-i-0e4dfcc8c2daeb569-1\xgboost\xgboost-ci-windows\src\data\ellpack_page.cu(174): error : class "cuda::std::__4::tuple<size_t, size_t, size_t>" has no member "get" [C:\buildkite-agent\builds\buildkite-windows-cpu-autoscaling-group-i-0e4dfcc8c2daeb569-1\xgboost\xgboost-ci-windows\build\src\objxgboost.vcxproj] �_bk;t=1714406066901� auto e = batch.GetElement(out.get<2>());

But I do not think it is part of this PR?

@ZiyueXu77
Copy link
Author

And the other two errors are

File "/__w/xgboost/xgboost/tests/ci_build/test_r_package.py", line 166, in check_rpackage with open(rcheck_dir / "00install.out", "r") as fd: ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FileNotFoundError: [Errno 2] No such file or directory: 'xgboost.Rcheck/00install.out'

�_bk;t=1714406293923�An error occurred (RepositoryAlreadyExistsException) when calling the CreateRepository operation: The repository with name 'xgb-ci.clang_tidy11.8' already exists in the registry with id '492475357299'

I don't quite get what they indicate.

@trivialfis
Copy link
Member

Let's ignore the Windows error for now and get the Linux buildkite ci to pass: https://buildkite.com/xgboost/xgboost-ci/builds/5116#018f2a11-c525-41a1-a9d7-81bfdb1f701c

@ZiyueXu77
Copy link
Author

Let's ignore the Windows error for now and get the Linux buildkite ci to pass: https://buildkite.com/xgboost/xgboost-ci/builds/5116#018f2a11-c525-41a1-a9d7-81bfdb1f701c

Thanks! addressed the warning in clang-tidy

@trivialfis
Copy link
Member

Excellent! Please continue to fix other errors in the Linux CI (at the moment, it's the memory leak reported by the leak sanitizer).

@ZiyueXu77
Copy link
Author

Linux buildkite ci passed with LeakSanitizer fix :)

@trivialfis trivialfis merged commit 8585df5 into dmlc:vertical-federated-learning May 16, 2024
25 of 28 checks passed
@ZiyueXu77 ZiyueXu77 deleted the SecureBoostInf branch May 17, 2024 20:36
trivialfis pushed a commit to trivialfis/xgboost that referenced this pull request Jul 2, 2024
trivialfis added a commit that referenced this pull request Jul 2, 2024
trivialfis added a commit to trivialfis/xgboost that referenced this pull request Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants