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

Add a new KvikIO compatibility mode "AUTO" #547

Merged
merged 52 commits into from
Nov 20, 2024
Merged

Add a new KvikIO compatibility mode "AUTO" #547

merged 52 commits into from
Nov 20, 2024

Conversation

kingcrimsontianyu
Copy link
Contributor

@kingcrimsontianyu kingcrimsontianyu commented Nov 8, 2024 •
edited
Loading

This PR addresses issue #529. A new compatibility mode AUTO (previously proposed as ALLOW) is added that explicitly asks KvikIO to determine whether ON or OFF is eventually used for the I/O.

Closes #529

@kingcrimsontianyu kingcrimsontianyu self-assigned this Nov 8, 2024
@kingcrimsontianyu kingcrimsontianyu added breaking Introduces a breaking change c++ Affects the C++ API of KvikIO feature request New feature or request non-breaking Introduces a non-breaking change and removed breaking Introduces a breaking change labels Nov 8, 2024
@kingcrimsontianyu kingcrimsontianyu changed the title Add a new option to KvikIO compatibility mode Add a new option "ALLOW" to KvikIO compatibility mode Nov 8, 2024
@kingcrimsontianyu kingcrimsontianyu force-pushed the improve-compat-mode-definition branch from 69660c6 to 6557e56 Compare November 8, 2024 21:54
@kingcrimsontianyu kingcrimsontianyu changed the title Add a new option "ALLOW" to KvikIO compatibility mode Add a new KvikIO compatibility mode "AUTO" Nov 9, 2024
@kingcrimsontianyu kingcrimsontianyu force-pushed the improve-compat-mode-definition branch from 4884ccf to db89849 Compare November 10, 2024 04:59
madsbk
madsbk reviewed Nov 11, 2024
View reviewed changes
Copy link
Member

@madsbk madsbk left a comment

Choose a reason for hiding this comment

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

Looks great @kingcrimsontianyu but I am wondering if we should use the CompatMode enum instead of a string throughout?

@kingcrimsontianyu
Copy link
Contributor Author

kingcrimsontianyu commented Nov 11, 2024 •
edited
Loading

I'm still debating on this. My thought is that no matter what value the user specifies (via the environment variable or compat_mode_reset), internally defaults::_compat_mode will fall into one of the two states (ON/OFF) at the end, and the getter method bool compat_mode() never has a chance to return CompatMode::AUTO. So I kept CompatMode as only an internal detail. But I'm happy to make changes. Let me know what you think @madsbk @vuule . Thank you!

PS: If we decide to expose CompatMode in the public interface, do we need another getter method CompatMode requested_compat_mode() that returns the value user initially passed?

@madsbk
Copy link
Member

madsbk commented Nov 11, 2024

I have no strong opinion, but leaning towards exposing CompatMode::AUTO.

PS: If we decide to expose CompatMode in the public interface, do we need another getter method CompatMode requested_compat_mode() that returns the value user initially passed?

No, I don't think that is needed.

@kingcrimsontianyu kingcrimsontianyu added breaking Introduces a breaking change and removed non-breaking Introduces a non-breaking change labels Nov 12, 2024
@kingcrimsontianyu kingcrimsontianyu marked this pull request as ready for review November 12, 2024 15:53
@kingcrimsontianyu kingcrimsontianyu requested review from a team as code owners November 12, 2024 15:53
vuule
vuule reviewed Nov 12, 2024
View reviewed changes
cpp/include/kvikio/defaults.hpp Outdated Show resolved Hide resolved
cpp/src/file_handle.cpp Outdated Show resolved Hide resolved
cpp/include/kvikio/defaults.hpp Show resolved Hide resolved
madsbk
madsbk requested changes Nov 12, 2024
View reviewed changes
Copy link
Member

@madsbk madsbk left a comment

Choose a reason for hiding this comment

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

Looks great @kingcrimsontianyu! Could you also update the C++ docs and the Python docs ?

@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Nov 13, 2024

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.

@kingcrimsontianyu
Copy link
Contributor Author

/ok to test

1 similar comment
@kingcrimsontianyu
Copy link
Contributor Author

/ok to test

@kingcrimsontianyu
Copy link
Contributor Author

/ok to test

1 similar comment
@kingcrimsontianyu
Copy link
Contributor Author

/ok to test

@kingcrimsontianyu kingcrimsontianyu force-pushed the improve-compat-mode-definition branch from 5e38233 to ca4bf22 Compare November 15, 2024 18:48
@kingcrimsontianyu kingcrimsontianyu mentioned this pull request Nov 15, 2024
Open
@kingcrimsontianyu
Copy link
Contributor Author

Created #556 to improve our testing on GDS/POSIX I/O code in the future.

@kingcrimsontianyu kingcrimsontianyu marked this pull request as ready for review November 15, 2024 20:01
@kingcrimsontianyu kingcrimsontianyu marked this pull request as draft November 15, 2024 22:24
vuule
vuule approved these changes Nov 19, 2024
View reviewed changes
Copy link
Contributor

@vuule vuule left a comment

Choose a reason for hiding this comment

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

Great change! Just one optional suggestion.

cpp/include/kvikio/defaults.hpp Outdated Show resolved Hide resolved
@kingcrimsontianyu kingcrimsontianyu marked this pull request as ready for review November 19, 2024 20:16
@kingcrimsontianyu
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit 5ff93bc into rapidsai:branch-24.12 Nov 20, 2024
57 checks passed
@pxLi pxLi mentioned this pull request Nov 20, 2024
Closed
@kingcrimsontianyu kingcrimsontianyu mentioned this pull request Nov 20, 2024
Merged
3 tasks
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this pull request Nov 20, 2024
@kingcrimsontianyu
Adapt to KvikIO API change in the compatibility mode (#17377)
6f83b58 
This PR adapts cuDF to a breaking API change in KvikIO (rapidsai/kvikio#547) introduced recently, which adds the `AUTO` compatibility mode to file I/O.

This PR causes no behavioral changes in cuDF: If the environment variable `KVIKIO_COMPAT_MODE` is left unset, cuDF by default still enables the compatibility mode in KvikIO. This is the same with the previous behavior (#17185).

Authors:
  - Tianyu Liu (https://github.com/kingcrimsontianyu)

Approvers:
  - Vukasin Milovanovic (https://github.com/vuule)

URL: #17377
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wence- wence- wence- left review comments

@vuule vuule vuule approved these changes

@madsbk madsbk madsbk approved these changes

@msarahan msarahan Awaiting requested review from msarahan msarahan was automatically assigned from rapidsai/packaging-codeowners

Assignees

@kingcrimsontianyu kingcrimsontianyu

Labels
breaking Introduces a breaking change c++ Affects the C++ API of KvikIO feature request New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improvement on KVIKIO_COMPAT_MODE
4 participants
@kingcrimsontianyu @madsbk @vuule @wence-