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

Draft
wants to merge 44 commits into
base: branch-24.12
Choose a base branch
from
Draft

Add a new KvikIO compatibility mode "AUTO" #547

wants to merge 44 commits into from
+486 −117

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 changed the title Add a new option "ALLOW" to KvikIO compatibility mode Add a new KvikIO compatibility mode "AUTO" Nov 9, 2024
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
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
return; // Nothing to do in compatibility mode
} else if (_compat_mode == CompatMode::AUTO) {
_compat_mode = defaults::infer_compat_mode_from_runtime_sys(_compat_mode);
Copy link
Contributor

Choose a reason for hiding this comment

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

why not infer in _compat_mode init?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've brought the infer to the beginning of the constructor. Is this what you would recommend?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant the line _compat_mode{compat_mode}, but it looks like we don't want to infer at all to ensure correct fallback.

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 and others added 24 commits November 15, 2024 11:00
@kingcrimsontianyu
Cleanup
07f926b
@kingcrimsontianyu
Address reviewer comments
2bb0df8
@kingcrimsontianyu
Lint
e3193f1
@kingcrimsontianyu
Fix comment
9fb4ce3
@kingcrimsontianyu
Reset compat mode state for file handle
7eda453
@kingcrimsontianyu
Fix test
b652e87
@kingcrimsontianyu
Address review comment
3df9b25
@kingcrimsontianyu
Address reviewer comments
16425ee
@kingcrimsontianyu
Update the doc
445cacc
@kingcrimsontianyu
Improve comments
aecedc4
@kingcrimsontianyu
Remove compat inference from defautls
06ada9e
@kingcrimsontianyu
Address compat mode in buffer
c84d632
@kingcrimsontianyu
A happy update
b336039
@kingcrimsontianyu
Update examples
1e3b1a9
@kingcrimsontianyu
Cleanup
b5802f2
@kingcrimsontianyu
Update for the batch
e782e33
@kingcrimsontianyu
Fix copyright lint error
f9d302e
@kingcrimsontianyu
Fix docs
82c973e
@kingcrimsontianyu
Fix header
4f35a49
@kingcrimsontianyu
Fix Sphinx doc issue. Improve string parsing for compat mode
12b0a43
@jameslamb @kingcrimsontianyu
enforce wheel size limits, README formatting in CI (rapidsai#548)
7d8da5e 
Contributes to rapidsai/build-planning#110

Proposes adding 2 types of validation on wheels in CI, to ensure we continue to produce wheels that are suitable for PyPI.

* checks on wheel size (compressed),
  - *to be sure they're under PyPI limits*
  - *and to prompt discussion on PRs that significantly increase wheel sizes*
* checks on README formatting
  - *to ensure they'll render properly as the PyPI project homepages*
  - *e.g. like how https://github.com/scikit-learn/scikit-learn/blob/main/README.rst becomes https://pypi.org/project/scikit-learn/*

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Bradley Dice (https://github.com/bdice)

URL: rapidsai#548
@kingcrimsontianyu
Rename compat mode related functions
b94f30f
@kingcrimsontianyu
Fix compat mode. Update doc
1852545
@kingcrimsontianyu
Futher tweaks
0725d63
@kingcrimsontianyu kingcrimsontianyu marked this pull request as draft November 15, 2024 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vuule vuule vuule left review comments

@wence- wence- wence- left review comments

@madsbk madsbk madsbk approved these changes

@msarahan msarahan Awaiting requested review from msarahan msarahan is a code owner automatically assigned from rapidsai/packaging-codeowners

At least 1 approving review is required to merge this pull request.

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
5 participants
@kingcrimsontianyu @madsbk @vuule @wence- @jameslamb