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 a new KvikIO compatibility mode "AUTO" #547

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

Conversation

kingcrimsontianyu
Copy link
Contributor

@kingcrimsontianyu kingcrimsontianyu commented Nov 8, 2024

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
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

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
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
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 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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Introduces a breaking change c++ Affects the C++ API of KvikIO feature request New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improvement on KVIKIO_COMPAT_MODE
4 participants