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.

Sign up for GitHub

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

Jump to bottom

Extract dask and spark test into distributed test. #8395

Merged
merged 13 commits into from Oct 28, 2022
Merged

Extract dask and spark test into distributed test. #8395

merged 13 commits into from Oct 28, 2022

Conversation

trivialfis
Copy link
Member

  • Move test files.
  • Run spark and dask separately to prevent conflicts.
  • Gather common code into testing module.

@trivialfis
Copy link
Member Author

Close #8134 .

@trivialfis trivialfis added this to To Do in PySpark Support via automation Oct 27, 2022
@trivialfis trivialfis changed the title [WIP] Extract dask and spark test into distributed test. Extract dask and spark test into distributed test. Oct 27, 2022
@trivialfis trivialfis requested a review from hcho3 October 27, 2022 13:21
trivialfis
trivialfis commented Oct 27, 2022
View reviewed changes
tests/test_distributed/test_federated/test_federated.py
run_test(with_ssl=False, with_gpu=False)
run_test(with_ssl=True, with_gpu=True)
run_test(with_ssl=False, with_gpu=True)
run_federated(with_ssl=True, with_gpu=False)
Copy link
Member Author

Choose a reason for hiding this comment

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

We should convert these into pytest and run them on CI.

@trivialfis
Copy link
Member Author

trivialfis commented Oct 27, 2022 •
edited

Should I put the tests as part of XGBoost module?
Pros:

  • Users can run the tests, especially helpful if they are troubleshooting distributed environment.
  • Distributors can run the tests. This way packaging systems like conda don't need to download the git repository or invent their own tests.
  • Easier imports when writing tests.

Cons:

  • The size of the Python package will grow.
  • We need some heavy changes to annotate the tests with Python type hints and make the linter happy.
  • Might have wrong imports.

Some other libraries like dask/distributed, cudf, and cuml are integrating the tests into their Python module. For more implications, see https://docs.pytest.org/en/7.1.x/explanation/goodpractices.html#choosing-a-test-layout-import-rules .

@trivialfis
Copy link
Member Author

Let's keep the existing layout for now. If the need ever comes up we can make the change then, git can recognize moving files.

@trivialfis trivialfis merged commit cfd2a9f into dmlc:master Oct 28, 2022
@trivialfis trivialfis deleted the move-tests branch October 28, 2022 08:24
@trivialfis trivialfis moved this from To Do to Done in PySpark Support Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hcho3 hcho3 hcho3 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
PySpark Support
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@trivialfis @hcho3