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

Add tests for Resultset, some refactoring of Resultset and Dataset #3857

Closed
wants to merge 30 commits into from

Conversation

betochimas
Copy link
Contributor

@betochimas betochimas commented Sep 12, 2023

This PR primarily adds testing for the Resultset class, introduced earlier in 23.10. The tests take a similar approach to test_dataset, creating a temporary directory to test downloading all result files. To align Resultset and Dataset, the setter and getter for each download directory is moved into DefaultDownloadDir, so that each class shares an instance of DefaultDownloadDir and can be configured independently, although their default locations are still both dependent on the RAPIDS_DATASET_ROOT_DIR_PATH environment variable. The old patterns are present but commented-out, so this change would be breaking.

@copy-pr-bot
Copy link

copy-pr-bot bot commented Sep 12, 2023

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@betochimas
Copy link
Contributor Author

Remaining points to address and relevant info for whoever picks this up:

  • Moves set_download_dir and get_download_dir into DefaultDownloadDir class
  • Both default_download_dir and default_resultset_download_dir are instances of DefaultDownloadDir
  • Adds test_resultset.py which is compatible, created with the above refactoring in mind
  • Old Dataset and Resultset helpers are commented out in case the above refactoring is scrapped / moved to another PR
  • Passes CI without issue as it stands

@BradReesWork BradReesWork added this to the 23.10 milestone Sep 25, 2023
@nv-rliu nv-rliu self-assigned this Sep 26, 2023
@BradReesWork BradReesWork changed the base branch from branch-23.10 to branch-23.12 September 26, 2023 14:16
@BradReesWork BradReesWork added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Sep 26, 2023
@BradReesWork BradReesWork removed this from the 23.10 milestone Sep 26, 2023
@BradReesWork BradReesWork added this to the 23.12 milestone Sep 26, 2023
@nv-rliu nv-rliu closed this Oct 26, 2023
rapids-bot bot pushed a commit that referenced this pull request Nov 19, 2023
This PR replaces and is a continuation of #3857 (by @betochimas)

> This PR primarily adds testing for the `Resultset` class, introduced earlier in 23.10. The tests take a similar approach to test_dataset, creating a temporary directory to test downloading all result files. To align `Resultset` and `Dataset`, the setter and getter for each download directory is moved into `DefaultDownloadDir`, so that each class shares an instance of `DefaultDownloadDir` and can be configured independently, although their default locations are still both dependent on the RAPIDS_DATASET_ROOT_DIR_PATH environment variable. The old patterns are present but commented-out, so this change would be breaking. 

This PR also removes the deprecated `experimental.datasets` package due to it being promoted to stable for >=1 release.

Authors:
  - Ralph Liu (https://github.com/nv-rliu)
  - Dylan Chima-Sanchez (https://github.com/betochimas)
  - Rick Ratzel (https://github.com/rlratzel)
  - Brad Rees (https://github.com/BradReesWork)

Approvers:
  - Rick Ratzel (https://github.com/rlratzel)

URL: #3957
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants